feat(examples): add Polymarket copytrading bot #65
Conversation
Add a copytrading bot that monitors a target wallet's trades and mirrors them on your account. Features include: - Poll-based trade monitoring via Data API - Configurable scale factor for position sizing - Market filtering by slug - Duplicate trade prevention - Status logging with session statistics
|
Code Review Summary: STRENGTHS:
CODE QUALITY ISSUES:
BUGS:
PERFORMANCE:
SECURITY:
TEST COVERAGE:
RECOMMENDATION: Request changes to align with CLAUDE.md Rule 4 and add input validation. Overall: Solid code with good structure. With minor adjustments this will be a valuable addition. |
Add TelegramBot utility class for sending notifications via Telegram Bot API. Integrate with copytrading bot to send: - Startup/shutdown notifications - Trade detected and copied notifications - Error notifications Usage: --telegram --telegram-token <token> --telegram-chat-id <id> Or set TELEGRAM_BOT_TOKEN and TELEGRAM_CHAT_ID environment variables
PR Review: Polymarket Copytrading BotSummaryThis PR adds a copytrading bot for Polymarket with Telegram notifications. The implementation is well-structured and follows the codebase patterns. However, there are several important issues to address. Critical Issues1. CLAUDE.md Violation: Emojis (Line 22, 144-149, dr_manhattan/utils/logger.py:21)The codebase guidelines explicitly state: "Avoid emojis and other non-essential characters." Violations:
Action Required: Remove all emojis from the Telegram notification messages. Use text indicators instead: side_symbol = "[BUY]" if side.upper() == "BUY" else "[SELL]"
text = f"{side_symbol} Trade {action}\n..."2. CLAUDE.md Violation: Hardcoded Values vs Single Source of TruthPer guideline #4: "DO NOT place many variables in .env file. Place them in the code instead." Issue: The code uses environment variables for configuration that should be constants:
Recommendation: Define these as class-level constants or module constants: # At module level
DEFAULT_SCALE_FACTOR = 1.0
DEFAULT_POLL_INTERVAL = 5.0
DEFAULT_MAX_POSITION = 100.0
DEFAULT_MIN_TRADE_SIZE = 1.0Security Concerns3. No Request Timeout Default in TelegramBot.init
Recommendation: Add validation: self.timeout = min(timeout, 30) # Cap at 30 seconds4. Wallet Address Truncation in Logs May Cause Confusion
Recommendation: Include full address in logs, only truncate in UI-facing messages. 5. No Validation of Target Wallet Address Format
Recommendation: Add basic validation: if not target_wallet.startswith('0x') or len(target_wallet) != 42:
raise ValueError(f"Invalid wallet address: {target_wallet}")Code Quality & Best Practices6. Missing Error Context in Exception HandlersMultiple locations use generic exception handling without preserving context:
Recommendation: Log the exception type and use more specific exception handling: except requests.RequestException as e:
logger.warning(f"Network error fetching market {condition_id}: {type(e).__name__}: {e}")
except Exception as e:
logger.error(f"Unexpected error: {type(e).__name__}: {e}", exc_info=True)7. Inconsistent Type Annotations
Recommendation: Consider using TypedDict or dataclass for response types: from typing import TypedDict
class TelegramResponse(TypedDict, total=False):
ok: bool
result: dict
description: str8. Magic Strings for Side Comparison
Recommendation: Create an enum or use the existing OrderSide enum consistently: is_buy = trade.side.upper() == OrderSide.BUY.value9. Potential Memory Leak in copied_trades Set
Recommendation: Implement a bounded cache or time-based expiration: from collections import deque
self.copied_trades: deque = deque(maxlen=10000) # Keep last 10k trades10. No Graceful Shutdown Handling
Recommendation: Add signal handlers: import signal
def shutdown_handler(signum, frame):
logger.info("Received shutdown signal")
self.stop()
signal.signal(signal.SIGTERM, shutdown_handler)
signal.signal(signal.SIGINT, shutdown_handler)Performance Considerations11. Inefficient Market Lookup
Current: Cache by condition_id only 12. Poll Interval Too Aggressive for Default
Recommendation: Increase default to 10-15 seconds or add exponential backoff on errors. 13. Synchronous Requests in Telegram Module
Impact: During Telegram API slowdowns, the bot's trade monitoring is delayed. Recommendation: Consider async implementation or at least use a thread pool for notifications. Test Coverage14. No Tests ProvidedThis PR adds 723 lines of code but no corresponding tests. Required Tests:
Example test structure: # tests/test_copytrading.py
import pytest
from unittest.mock import Mock, patch
from examples.copytrading import CopytradingBot
def test_should_copy_trade_filters_duplicates():
# Test that duplicate trades are skipped
...
def test_telegram_send_handles_timeout():
# Test that telegram errors don't crash bot
...15. No Documentation for SetupWhile the docstring shows usage, there's no documentation for:
Recommendation: Add a section to README or create Minor Issues16. Inconsistent String FormattingMix of f-strings and .format() - prefer f-strings throughout for consistency. 17. Unused Imports Check
18. Hardcoded Color in Logger
Positive Aspects✓ Well-structured class design with clear separation of concerns Recommendations SummaryMust Fix (Blocking):
Should Fix (High Priority): Nice to Have: Overall Assessment: Good implementation with solid architecture, but needs to comply with CLAUDE.md guidelines (especially emoji removal) and add test coverage before merging. |
Refactor Telegram module into a scalable folder structure: - telegram/types.py: Strict type definitions (ParseMode, SendResult, etc.) - telegram/bot.py: Generic TelegramBot class with type-safe API - telegram/formatters.py: HTML formatting utilities and MessageBuilder Features: - Fully typed with dataclasses and enums - Generic bot class for any use case - MessageBuilder for fluent message construction - HTML formatting helpers (bold, code, link, etc.) - Support for photos, documents, inline keyboards - Batch sending and message editing
Pull Request Review: Polymarket Copytrading BotOverviewThis PR adds a copytrading bot for Polymarket and a comprehensive Telegram integration module. The implementation demonstrates good software engineering practices with well-structured code and clear separation of concerns. Strengths
Critical Issues1. Hardcoded Emojis Violate CLAUDE.md
2. Race Condition in Trade Polling
3. Duplicate Code in MessageBuilder.field()
4. Missing Test Coverage
Medium Priority Issues5. Environment Variables vs Code (CLAUDE.md)
6. Missing Input Validation
7. Potential Memory Leak
8. Inconsistent Error Notifications
Security ConcernsThe bot executes real financial transactions automatically. Consider adding:
Recommendations SummaryMust Fix Before Merge:
Should Fix:
Overall AssessmentWell-written code with good architecture and type safety. Main concerns are CLAUDE.md compliance, missing tests, and production safety features. Recommendation: Request changes to address critical issues before merging. The Telegram module is genuinely reusable and well-designed. The copytrading bot needs safety guardrails before production use. |
Refactor copytrading bot into a modular folder structure: - copytrading/types.py: Type definitions (BotConfig, CopyStats, TradeInfo) - copytrading/notifications.py: Pluggable notification system - copytrading/bot.py: Core CopytradingBot class - copytrading/cli.py: Command-line interface - copytrading/__main__.py: Module entry point Features: - Clean separation of concerns - Pluggable notification handlers (Telegram, Null) - Type-safe configuration with validation - Programmatic and CLI usage supported
PR Review: Polymarket Copytrading BotOverviewThis PR adds a comprehensive copytrading bot for Polymarket with Telegram notifications. The implementation is well-structured with good separation of concerns across 11 files (1,773 additions). Code Quality & Best PracticesStrengths
Issues & Suggestions1. Configuration Management (CRITICAL - Violates CLAUDE.md #4)The CLI heavily uses environment variables, violating the "Single Source of Truth" principle: cli.py:1455-1475 - Multiple environment variable fallbacks default=float(os.getenv("SCALE_FACTOR", "1.0"))
default=float(os.getenv("POLL_INTERVAL", "5"))
default=float(os.getenv("MAX_POSITION", "100"))
# ... etcRecommendation: Remove environment variable defaults from CLI args. Keep only sensitive credentials (API keys) in env. Put defaults directly in 2. Hardcoded Valuesbot.py:1254 - Hardcoded limit of 50 trades: trades = self._exchange.fetch_public_trades(
user=self._config.target_wallet,
limit=50, # Should be configurable
taker_only=True,
)Recommendation: Add 3. Missing Input Validationcli.py:1452-1506 - No validation for:
Recommendation: Add validation in 4. Error Handling Gapsbot.py:1224-1232 - Order creation has no retry logic: try:
order = self._exchange.create_order(...)
except Exception as e:
logger.error(f"Failed to execute copy trade: {e}")
return FalseRecommendation: Add configurable retry logic for network failures vs. validation errors. Security ConcernsCRITICAL Issues1. Private Key Exposure Riskcli.py:1512-1513 - Private keys loaded from environment: private_key = os.getenv("POLYMARKET_PRIVATE_KEY") or os.getenv("PRIVATE_KEY")The code properly keeps credentials in env vars (good), but there's no guidance on secure key management in the docstring or examples. Recommendation: Add security warnings to the module docstring about:
2. No Rate LimitingThe bot has no protection against API rate limits. Aggressive polling could lead to:
Recommendation: Add rate limiting with exponential backoff on errors. 3. Input Sanitizationformatters.py:494-547 - HTML escape is used, but not consistently enforced. Some functions call Recommendation: Ensure all user-controlled input to Telegram is escaped. 4. No Maximum Position Limits at Exchange LevelThe code caps position size client-side, but doesn't verify account balance before trading. Recommendation: Add pre-trade balance checks to prevent failed orders. Performance ConsiderationsIssues1. Inefficient Market Cachingbot.py:1148-1171 - Cache never expires, could grow unbounded: self._market_cache: Dict[str, Market] = {}Recommendation: Add TTL-based cache expiration or LRU cache with size limit. 2. Synchronous I/O in Loopbot.py:1377-1387 - Blocking network calls in main loop: while self._is_running:
trades = self._poll_trades() # Blocks
if trades:
self._process_trades(trades) # Blocks on order creation
time.sleep(self._config.poll_interval)Impact: If order creation takes 3s and polling interval is 5s, effective poll rate is 8s. Recommendation: Use async/await or thread pool for parallel order execution. 3. Redundant API Callsbot.py:1158-1164 - Fetches all markets by slug when only one is needed: markets = self._exchange.fetch_markets_by_slug(slug)
for market in markets:
self._market_cache[market.id] = marketRecommendation: Directly fetch by condition_id first, only fall back to slug search. Bugs & Logic IssuesCRITICAL1. Race Condition on Startupbot.py:1259-1261 - First poll has no timestamp filter: if self._last_poll_time:
trades = [t for t in trades if t.timestamp > self._last_poll_time]Impact: On startup, bot fetches last 50 trades and may copy old trades if they're not already in Recommendation: Initialize 2. Trade Deduplication Relies on In-Memory Setbot.py:1091 - self._copied_trades: Set[str] = set()Impact: If bot crashes and restarts, it will re-copy recent trades. Recommendation: Persist copied trade IDs to disk/database. MODERATE3. Token ID Lookup Can Fail Silentlybot.py:1173-1190 - Returns token_id = self._get_token_id(market, outcome)
if not token_id:
logger.error(f"Cannot find token ID for outcome: {outcome}")
return FalseThis is handled correctly, but the error message doesn't include market context. 4. Telegram Message Truncation Not HandledTelegram has a 4096 character limit. Large messages will fail silently. Recommendation: Add message length checks and truncation/splitting logic. Test CoverageMissing TestsNo tests are included for:
Recommendation: Add unit tests for at least:
Best Practices Alignment with CLAUDE.md
Recommendations PriorityMust Fix (Before Merge)
Should Fix (Next PR)
Nice to Have
SummaryThis is a well-architected feature with clean separation of concerns and good type safety. However, it has several critical issues:
Recommendation: Request changes to address critical issues before merge. |
Pull Request Review: Polymarket Copytrading BotThank you for this comprehensive contribution! This PR adds a well-structured copytrading bot with Telegram integration. Here's my detailed review: ✅ StrengthsArchitecture & Code Quality
Following Project Standards
🔴 Critical Issues1. Security: Telegram Bot Token Exposure RiskLocation: The bot token is included directly in API URLs via string formatting: BASE_URL = "https://api.telegram.org/bot{token}/{method}"Risk: If any error logging or debugging exposes the URL, the bot token will be leaked. Recommendation: Never include secrets in URLs. Consider sanitizing logs or using POST body authentication where possible. Add warnings in documentation about not logging URLs. 2. Missing HTTP Status Code ValidationLocation: response = requests.post(url, json=data, timeout=self._config.timeout)
result = response.json()The code doesn't check Fix: Add 3. Race Condition in Duplicate Trade DetectionLocation: if trade_id in self._copied_trades: # Line 96
return False
# ... later ...
self._copied_trades.add(trade_info.trade_id) # Line 251If a trade execution fails after the check but before adding to the set, the same trade could be attempted multiple times on subsequent polls. Fix: Add to if trade_id in self._copied_trades:
return False
self._copied_trades.add(trade_id) # Mark as processed immediately4. Unbounded Memory GrowthLocation: self._copied_trades: Set[str] = set()
self._market_cache: Dict[str, Market] = {}Both sets/dicts grow unbounded during long-running sessions. Recommendation: Implement LRU cache for markets or add periodic cleanup. For copied trades, consider time-based expiration (e.g., trades older than 24h).
|
| Category | Rating | Notes |
|---|---|---|
| Code Quality | 8/10 | Well-structured, clean, type-safe |
| Security | 6/10 | Some concerns around token handling and input validation |
| Performance | 7/10 | Good caching, but some optimizations needed |
| Test Coverage | 2/10 | No tests - critical gap |
| Documentation | 7/10 | Good docstrings, could use more examples |
| CLAUDE.md Compliance | 7/10 | Violates rule #4 on environment variables |
✅ Recommended Action Items
Before Merge (High Priority):
- ✅ Fix race condition in duplicate trade detection
- ✅ Add HTTP status code checking to Telegram requests
- ✅ Remove env var defaults for non-secret config (CLAUDE.md fix: polymarket impl #4)
- ✅ Add input validation for environment variables
- ✅ Implement bounds checking for outcome_index access
After Merge (Medium Priority):
- Add unit tests for core logic
- Implement memory cleanup for long-running sessions
- Add rate limiting to Telegram bot
- Document slippage risks
Future Enhancements (Low Priority):
- Add signal handlers for graceful shutdown
- Optimize market lookup logic
- Consider adjustable status log frequency
🎉 Conclusion
This is a solid contribution with excellent architecture and clean code. The modular design makes it easy to extend and maintain. However, the lack of tests and some security/stability concerns should be addressed before merging to production.
The code quality is high, but financial trading bots require extra scrutiny. I recommend addressing the critical issues and adding basic test coverage before merging.
Recommendation: Request Changes - Address critical issues #1-4, then approve.
Great work overall! The Telegram integration is particularly well-designed and could be useful across other parts of the project. 🚀
Add a copytrading bot that monitors a target wallet's trades and mirrors
them on your account. Features include: