Implement Predict.fun exchange integration

[0xGh-st]

close #43

• Add Predict.fun exchange configuration to .env.example
• Update pyproject.toml and uv.lock to include predict-sdk dependency
• Enhance ExchangeClient and ExchangeConfig to support Predict.fun
• Introduce PredictFun class for handling API interactions with Predict.fun
• Update strategy to manage insufficient funds and improve order handling

This integration allows users to trade on the Predict.fun platform, enhancing the overall functionality of the trading system.

[claude]

Pull Request Review: Predict.fun Exchange Integration

Summary

This PR adds comprehensive support for the Predict.fun exchange on BNB Chain, including both EOA and Smart Wallet modes. The implementation is substantial (~1500 lines) and introduces complex cryptographic signing logic.

---

Code Quality & Best Practices

Strengths

1. Well-documented code: Clear docstrings and inline comments explaining complex logic
2. Comprehensive error handling: Proper exception types (InsufficientFunds, AuthenticationError, etc.)
3. Modular design: Good separation between authentication, order signing, and API communication
4. Configuration flexibility: Supports both testnet/mainnet and EOA/Smart Wallet modes

Issues & Concerns

🔴 CRITICAL: Violation of CLAUDE.md Guidelines

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

The PR adds 7 new environment variables to .env.example:

PREDICTFUN_API_KEY
PREDICTFUN_PRIVATE_KEY
PREDICTFUN_SMART_WALLET_OWNER_PRIVATE_KEY
PREDICTFUN_USE_SMART_WALLET
PREDICTFUN_SMART_WALLET_ADDRESS
PREDICTFUN_TESTNET

Recommendation: Consider reducing the number of env vars or refactoring to use configuration discovery/defaults where possible. At minimum, PREDICTFUN_TESTNET and PREDICTFUN_USE_SMART_WALLET could be inferred or use hardcoded defaults.

🟡 Code Duplication

• Lines 42-52: Multiple contract addresses as module-level constants. Consider using a dictionary structure:

  CONTRACTS = {
      "mainnet": {
          "yield_bearing_ctf": "0x6bEb5a40C032AFc305961162d8204CDA16DECFa5",
          "yield_bearing_neg_risk_ctf": "0x8A289d458f5a134bA40015085A8F50Ffb681B41d",
          # ... etc
      },
      "testnet": { ... }
  }

🟡 Magic Numbers

• Line 1056: max_approval = 2**256 - 1 - Should be a named constant
• Line 1058: approval_threshold = int(1e24) - Should be a named constant with comment explaining "1M USDT"
• Line 1182: max_salt = 2147483648 - Should be a named constant
• Line 1187: precision = int(1e13) - Defined in multiple places (lines 987, 1187)

🟡 Error Handling Issues

• Lines 879-880: Silently catching all exceptions and returning empty orderbook

  except Exception:
      return {"bids": [], "asks": []}

  Should at least log the error for debugging.

• Lines 1105-1108: Approval failures are only printed if verbose, but this is critical functionality

  except Exception as e:
      if self.verbose:
          print(f"Failed to set approvals: {e}")
      return False

  Should always log errors, not just in verbose mode.

---

Security Concerns

🔴 HIGH: Private Key Handling

• Lines 183-189: Private keys stored in memory as class attributes

  if self.private_key:
      self._account = Account.from_key(self.private_key)

  Recommendation: Consider using secure memory practices, clear sensitive data after use, or use hardware wallet integration where possible.

🟡 MEDIUM: Unlimited Token Approval

• Lines 1055-1056: Approves 2**256 - 1 tokens to exchange contracts

  max_approval = 2**256 - 1

  While common practice, this exposes users to risk if exchange contracts are compromised. Consider:
  • Warning users about this in documentation
  • Implementing a configurable approval amount
  • Time-limited approvals

🟡 MEDIUM: Timestamp Validation

• Line 1213: Orders set to expire in year 2100

  expiration = 4102444800  # 2100-01-01

  No validation that this is acceptable to the API. Should handle potential API rejections.

🟢 GOOD: Signature Implementation

• EIP-712 signing appears correctly implemented with proper domain separation
• Smart wallet Kernel domain wrapping follows the pattern correctly

---

Performance Considerations

🟡 Inefficient Lookups

• Lines 800-815: _find_market_by_slug_fallback performs O(n) search through all markets

  all_markets = self.fetch_markets({"all": True})
  for market in all_markets:
      if all(k in text for k in keywords):

  Could be improved with caching or pagination.

🟡 Blocking Web3 Calls

• Lines 1066-1092: Approval checking and transaction sending are synchronous
  • Could cause timeouts for users with slow RPC connections
  • Consider adding timeout parameters or async alternatives

🟢 GOOD: Session Reuse

• Line 160: Properly uses requests.Session() for connection pooling
• Line 180: _approvals_checked flag prevents redundant approval checks

---

Potential Bugs

🔴 Type Inconsistency

• dr_manhattan/base/exchange_client.py:787: Adding USDT to cash calculation

  cash = balance.get("USDC", 0.0) + balance.get("USD", 0.0) + balance.get("USDT", 0.0)

  This assumes USDT and USDC have the same value (1:1 peg). Should document this assumption or normalize to a single currency.

🟡 Potential Division by Zero

• Lines 850-873: Price inversion for second outcome

  inverted_price = 1.0 - float(entry[0])
  if inverted_price > 0:  # Only filters > 0, allows very small values

  Edge case: What if entry[0] is exactly 1.0? Results in inverted_price = 0.0, which passes the check but creates invalid orderbook entries.

🟡 OutcomeToken Model Change

• dr_manhattan/base/strategy.py:125: Changed OutcomeToken initialization

  OutcomeToken(market_id=self.market_id, outcome=outcome, token_id=token_id)

  This changes the signature. Need to verify this doesn't break existing exchanges (Polymarket, Opinion, Limitless).

🟡 Fallback Price Logic

• dr_manhattan/base/strategy.py:166-175: Complex fallback for price calculation
  Could fail silently if neither market.prices nor orderbook have data, resulting in price = 0.

---

Test Coverage

🔴 CRITICAL: No Unit Tests

• No test files found for PredictFun exchange (find tests -name "*predict*" returned nothing)
• Only minor test change in tests/mcp/test_exchange_tools.py (import update)

Recommendations:

1. Add unit tests for:
   • Authentication (both EOA and Smart Wallet modes)
   • EIP-712 signing logic
   • Order creation and cancellation
   • Orderbook fetching and price inversion
   • Error handling (insufficient funds, rate limits, etc.)
2. Add integration tests with testnet
3. Mock Web3 calls for deterministic testing

---

Architecture & Integration

🟢 GOOD: Follows Existing Patterns

• Properly extends Exchange base class
• Implements all required methods consistently
• Uses existing error types from base.errors

🟡 Strategy Changes Impact All Exchanges

• dr_manhattan/base/strategy.py: Changes to insufficient funds handling and OutcomeToken model affect all exchanges
• Should verify these changes don't break Polymarket, Opinion, Limitless

🟢 GOOD: Factory Pattern Integration

• Properly registered in exchange_factory.py
• Config dataclass follows existing pattern

---

Documentation

Missing Documentation

1. No README or usage guide for Predict.fun integration
2. No migration guide for differences between EOA and Smart Wallet modes
3. No security considerations documented (unlimited approvals, key storage, etc.)
4. API rate limits not documented (though RateLimitError is used)

Recommendations

1. Add a docs/exchanges/predictfun.md guide covering:
   • Setup for both modes
   • Security best practices
   • Troubleshooting common issues
2. Document the wallet modes in code comments more clearly
3. Add inline warnings about security implications

---

Summary & Recommendations

Must Fix Before Merge

1. ❌ Add comprehensive unit tests (critical for 1500+ lines of new code)
2. ❌ Address CLAUDE.md violation - reduce environment variables or get explicit approval to deviate from guidelines
3. ❌ Fix silent error handling - ensure critical errors are logged
4. ❌ Verify OutcomeToken changes don't break existing exchanges

Should Fix Before Merge

1. ⚠️ Refactor duplicate constants (contracts, precision values)
2. ⚠️ Add proper logging instead of print statements
3. ⚠️ Document security implications of unlimited approvals
4. ⚠️ Handle edge cases in price inversion logic

Nice to Have

1. 💡 Add async support for Web3 calls
2. 💡 Implement configurable approval amounts
3. 💡 Add usage documentation
4. 💡 Optimize market search with caching

---

Overall Assessment

Code Quality: 7/10 - Well-structured but needs refinement
Security: 6/10 - Standard practices but some concerns
Test Coverage: 2/10 - Critical gap
Documentation: 4/10 - Minimal

Recommendation: REQUEST CHANGES - The integration is solid, but requires tests and fixes before merging. The codebase guideline violation needs to be addressed.

---

Review generated with attention to security, performance, and maintainability. Please address critical issues before merging.

[claude]

Pull Request Review: Predict.fun Exchange Integration

Summary

This PR adds comprehensive integration with Predict.fun, a prediction market on BNB Chain. The implementation adds ~1,490 lines of new code in predictfun.py along with supporting configuration changes.

---

Code Quality & Best Practices

Strengths

1. Comprehensive Documentation: Excellent docstrings throughout the codebase explaining complex concepts (EIP-712 signing, smart wallet vs EOA modes)
2. Consistent Architecture: Follows existing patterns from other exchange integrations (Polymarket, Opinion, Limitless)
3. Error Handling: Proper use of custom exception types (InsufficientFunds, AuthenticationError, etc.)
4. Dual Mode Support: Well-implemented support for both EOA and Smart Wallet modes with clear separation of concerns

Areas for Improvement

1. Missing Test Coverage

• No unit tests for the new PredictFun class
• No integration tests
• Existing exchanges (Opinion, Polymarket, Limitless) all have test files

Recommendation: Add tests/test_predictfun.py with basic property tests, configuration validation, mock-based API tests, and Smart wallet vs EOA mode switching tests.

Critical: Following CLAUDE.md rule #5 (Run and Debug yourself PROACTIVELY), this integration should have been tested before PR submission.

2. Code Organization - Excessive Length

The predictfun.py file is 1,490 lines, making it the largest single file in the codebase. Consider extracting signing logic and constants in future refactoring.

---

Potential Bugs & Issues

1. Authentication Bug in Smart Wallet Mode (Line 368)

The retry logic checks self._account but should check self._owner_account in Smart Wallet mode:

Current: if self.api_key and self._account:

Fix: has_credentials = self._account if not self._is_using_smart_wallet() else self._owner_account

2. Hardcoded Magic Numbers (Lines 989, 1184)

PRECISION, MAX_SALT, and expiration should be module-level constants with explanatory comments.

3. Silent Failure in Balance Fetching (Line 1452)

Returning 0.0 on exception could mask serious issues (RPC down, wrong address, etc.). Consider logging warnings even when not verbose.

---

Security Concerns

1. Private Key Handling

• Keys stored as plain strings in memory
• No validation that keys start with 0x or are valid length
• No sanitization in error messages (keys could leak in logs)

Recommendation: Add private key validation.

2. Approval Transaction Gas Price (Line 1077)

Uses current network gas price without upper bound. During BNB Chain congestion, this could result in excessively expensive approval transactions.

Recommendation: Add configurable max gas price.

---

Changes to Shared Code

dr_manhattan/base/strategy.py

• Lines 149-152: USDT support addition - Good, backward compatible
• Lines 166-175: Fallback price calculation - Good, improves robustness
• Lines 529-545: Insufficient funds handling - Excellent graceful degradation

dr_manhattan/base/exchange_client.py

• Line 787: Added USDT to NAV calculation - Good for multi-exchange support

---

Final Recommendations

Must Fix Before Merge (P0)

1. Add test coverage - At minimum, basic unit tests
2. Fix authentication retry logic for Smart Wallet mode (line 368)

Should Fix Before Merge (P1)

3. Add private key validation
4. Extract magic numbers to module-level constants
5. Consider gas price limits for approval transactions

Nice to Have (P2)

6. Extract signing logic to separate module
7. Add documentation/example
8. Implement cache invalidation for token mappings

---

Overall Assessment

Code Quality: 4/5
Test Coverage: 1/5 - Major gap
Security: 3/5 - Good foundations, needs hardening
Documentation: 3/5 - Good inline docs, missing user docs
Performance: 4/5 - Well optimized

Recommendation: Approve with changes requested

This is a solid implementation that integrates a complex exchange with dual signing modes. The code quality is generally high and follows project conventions. However, the complete lack of tests and the authentication bug should be addressed before merging.

The integration appears feature-complete and ready for testing. With test coverage added and the authentication bug fixed, this would be a valuable addition to the project.