Skip to content

Comments

Adds PM CTF split/merge/redeem functionality#75

Open
minkyun12 wants to merge 1 commit intomainfrom
feat/redeem-split-merge
Open

Adds PM CTF split/merge/redeem functionality#75
minkyun12 wants to merge 1 commit intomainfrom
feat/redeem-split-merge

Conversation

@minkyun12
Copy link
Collaborator

Implements split, merge, and redeem operations for Conditional Token Framework (CTF) tokens on Polymarket.

Introduces new methods for splitting USDC into conditional tokens, merging them back, and redeeming winning tokens from resolved markets.

Leverages Polymarket's Builder API and relayer infrastructure for transaction submission and confirmation.

Adds helper functions for encoding transaction data and signing Safe transactions.

Implements split, merge, and redeem operations for Conditional Token Framework (CTF) tokens on Polymarket.

Introduces new methods for splitting USDC into conditional tokens, merging them back, and redeeming winning tokens from resolved markets.

Leverages Polymarket's Builder API and relayer infrastructure for transaction submission and confirmation.

Adds helper functions for encoding transaction data and signing Safe transactions.
@claude
Copy link

claude bot commented Jan 25, 2026

Test comment

@claude
Copy link

claude bot commented Jan 25, 2026

Pull Request Review: CTF Split/Merge/Redeem Functionality

Summary

This PR adds comprehensive support for Conditional Token Framework (CTF) operations on Polymarket. The implementation is well-structured with proper authentication and transaction signing. However, there are several critical issues that must be addressed before merge.


Critical Issues (Must Fix)

1. Missing Dependencies

The code imports web3 and eth_abi but pyproject.toml doesn't list them as dependencies.

Fix Required: Add to pyproject.toml:

dependencies = [
    "web3>=6.0.0",
    "eth-abi>=4.0.0",
    # ... existing dependencies
]

2. CLAUDE.md Violation - Too Many .env Variables

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

  • BUILDER_API_KEY
  • BUILDER_SECRET
  • BUILDER_PASS_PHRASE

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

Recommendation: Reduce the number of environment variables or move these to code constants.

3. Missing Wiki Documentation

Per CLAUDE.md line 6, when integrating new exchange functionality, you must update wiki/exchanges/polymarket.md to document:

  • CTF operations (split/merge/redeem)
  • Builder API configuration
  • Usage examples
  • Error handling

4. No Test Coverage

The PR adds 636 lines of code but no tests. Required tests:

  • Unit tests for encoding functions
  • HMAC signature generation tests
  • Safe transaction hash computation tests
  • Mock tests for split/merge/redeem operations
  • Error case tests

High Priority Issues (Should Fix)

5. Input Validation Missing

The split() and merge() methods don't validate inputs:

  • No check that amount > 0
  • No validation that condition_id is a valid hex string
  • Could accept negative amounts

6. Poor Error Handling in Polling

_poll_transaction() at line 2220:

  • Silent exception handling (except Exception: pass)
  • No logging when timeout occurs
  • Returns None for both timeout and failure cases

7. HMAC Signature Brittle Implementation

Line 2144: str(body).replace("'", '"') is unreliable for complex nested data.

Fix: Use json.dumps(body, separators=(',', ':'), sort_keys=True) for deterministic JSON.

8. Nonce Race Condition

_get_safe_nonce() reads nonce from chain but doesn't cache/increment it. Multiple rapid transactions could use the same nonce and fail.


Code Quality Strengths

  1. Well-organized structure with clear section headers
  2. Comprehensive docstrings with usage examples
  3. Proper EIP-712 signature implementation
  4. Good Web3 instance caching
  5. Clean removal of duplicate constants

Additional Recommendations

Extract Magic Numbers

USDC_DECIMALS = 6
DEFAULT_MAX_POLLS = 20
DEFAULT_POLL_INTERVAL = 2.0
SAFE_V_ADJUSTMENT = 4

Add Helper Methods

def get_ctf_balance(self, condition_id: str) -> Dict[str, float]:
    """Get CTF token balances for a condition"""
    pass

def can_merge(self, condition_id: str) -> float:
    """Calculate maximum mergeable amount"""
    pass

Compliance with CLAUDE.md

Rule Status Notes
1. Clean, focused code ⚠️ Good structure, improvements needed
2. No unnecessary docs Missing required wiki documentation
4. Single source of truth Too many .env variables
5. Run and debug proactively ⚠️ No evidence of testing
6. Wiki documentation Must update polymarket.md

Conclusion

The implementation demonstrates solid understanding of CTF protocol and Safe transaction signing. The core logic is sound. However, the following must be addressed:

Blockers:

  • Missing dependencies (web3, eth-abi)
  • CLAUDE.md violations (env vars, missing wiki docs)
  • No test coverage
  • Missing input validation

Recommendation: ❌ Request changes before merge

Once critical issues are resolved, this will be a valuable addition to the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant