Better MX

[guzus]

MX stands for "MM experience"

[claude]

PR Review: Better MX (Market Maker Experience)

Summary

This PR introduces significant improvements to market maker experience with order fill tracking, NAV calculation, and enhanced WebSocket support. The changes add ~793 lines across 9 files.

Code Quality & Best Practices

✅ Strengths

1. Clean Architecture: New features follow existing patterns and integrate well with the base Exchange class
2. Good Documentation: Functions have clear docstrings with examples
3. Thread Safety: OrderTracker properly uses locks for concurrent access
4. Type Hints: Good use of type annotations (dataclasses, Callable types)
5. Error Handling: Callbacks wrapped in try-catch to prevent failures from propagating

⚠️ Areas for Improvement

1. Authentication Security (polymarket_ws.py:453-465)
The authentication implementation sends credentials without proper HMAC signing:

return {
    "auth": {
        "apiKey": self.api_key,
        "secret": self.api_secret,  # Secret sent directly
        "passphrase": self.api_passphrase,
        "timestamp": timestamp,
    }
}

Most WebSocket APIs require HMAC signatures, not raw secrets. Verify this matches Polymarket's actual auth requirements.

2. Float Comparison Issues (order_tracker.py:140, spread_strategy.py:361)

if tracked.total_filled >= tracked.order.size:  # Line 140
if abs(order.price - our_bid) < 0.001:  # Line 361

Using >= for float comparison can cause issues with floating-point precision. Consider using epsilon comparison like >= (size - 1e-9).

3. Missing Input Validation (base/exchange.py:76-84)

def update_mid_price(self, token_id: str, mid_price: float) -> None:
    self._mid_price_cache[str(token_id)] = mid_price

No validation that mid_price is in valid range [0, 1] for prediction markets. Invalid prices could corrupt NAV calculations.

4. Resource Cleanup (polymarket_ws.py:593-606)

def stop(self):
    self._running = False
    if self.ws and self._loop:
        async def close():
            if self.ws:
                await self.ws.close()
        asyncio.run_coroutine_threadsafe(close(), self._loop)

The coroutine is scheduled but not awaited. The thread join may complete before the WebSocket closes, leaving connections open.

Potential Bugs

1. Race Condition (order_tracker.py:129)

tracked.total_filled += trade.size

If multiple trades arrive simultaneously for the same order, this could cause double-counting since the lock is released between updates.

2. Memory Leak (base/exchange.py:50)

self._mid_price_cache: Dict[str, float] = {}

The mid-price cache never expires. In long-running bots, this could grow unbounded if markets are added/removed.

3. Infinite Loop Risk (polymarket_ws.py:492-510)

while self._running:
    try:
        if not self._connected:
            await self._connect()
        async for message in self.ws:
            # process message
    except Exception as e:
        await asyncio.sleep(3)

If _connect() consistently fails, this creates an infinite reconnection loop with no backoff limit.

4. Missing None Check (spread_strategy.py:313-318)
When calculating inverse prices for binary markets, if the first token's orderbook is missing, the code continues but best_bid/best_ask may be None, causing issues downstream.

Performance Considerations

1. WebSocket Efficiency: Smart optimization to only subscribe to Yes token for binary markets, calculating No as inverse. Saves 50% bandwidth. ✅

2. Cache TTL: Mid-price cache has no expiration, but this might be acceptable for real-time data that's constantly updated.

3. Lock Contention: OrderTracker uses a single lock for all operations. For high-frequency trading with many orders, consider finer-grained locking.

Security Concerns

CRITICAL: API credentials handling (polymarket.py:1346-1349)

creds = self._clob_client.creds
self._user_ws = PolymarketUserWebSocket(
    api_key=creds.api_key,
    api_secret=creds.api_secret,
    api_passphrase=creds.api_passphrase,
)

The PR passes API secrets to WebSocket without verification that the auth mechanism is secure. Ensure:

• Secrets are never logged (check verbose mode)
• WebSocket connection uses TLS
• Authentication method matches Polymarket's requirements

Test Coverage

Major Gap: No tests for new functionality

• No tests for OrderTracker fill detection
• No tests for NAV calculation edge cases
• No tests for WebSocket authentication
• No tests for mid-price cache updates

Per CLAUDE.md #5 ("Run and Debug yourself PROACTIVELY"), tests should be added.

Recommendations

Must Fix

1. Add input validation for update_mid_price() to ensure prices are in [0, 1]
2. Fix WebSocket cleanup to properly await connection closure
3. Add exponential backoff to reconnection logic
4. Verify authentication implementation matches Polymarket API docs

Should Fix

1. Add epsilon comparisons for float arithmetic
2. Implement cache eviction policy for mid-price cache
3. Add None checks when calculating inverse prices
4. Add basic unit tests for NAV calculation

Consider

1. Add metrics/monitoring hooks for order tracker
2. Document expected behavior when WebSocket disconnects mid-trade
3. Consider making reconnection behavior configurable

Verdict

The PR adds valuable functionality with generally good code quality. However, it has several bugs and missing safeguards that should be addressed before merging. The lack of tests is concerning given the complexity of WebSocket handling and order tracking.

Recommendation: Request changes - address critical issues (auth security, float comparisons, resource cleanup) and add basic tests before merging.