Feat/vw--more-stocks

[vince-taoshi]

Taoshi Pull Request

Description

[Provide a brief description of the changes introduced by this pull request.]

Related Issues (JIRA)

[Reference any related issues or tasks that this pull request addresses or closes.]

Checklist

• I have tested my changes on testnet.
• I have updated any necessary documentation.
• I have added unit tests for my changes (if applicable).
• If there are breaking changes for validators, I have (or will) notify the community in Discord of the release.

Reviewer Instructions

[Provide any specific instructions or areas you would like the reviewer to focus on.]

Definition of Done

• Code has been reviewed.
• All checks and tests pass.
• Documentation is up to date.
• Approved by at least one reviewer.

Checklist (for the reviewer)

• Code follows project conventions.
• Code is well-documented.
• Changes are necessary and align with the project's goals.
• No breaking changes introduced.

Optional: Deploy Notes

[Any instructions or notes related to deployment, if applicable.]

/cc @mention_reviewer

[github-actions]

🤖 Claude AI Code Review

Last reviewed on: 00:00:00

Summary

This PR introduces a major refactoring to expand equity trading support by adding ~1000 Russell 1000 stocks. Key changes include:

• Split TradePair from vali_config.py into dedicated trade_pair.py module
• Added bulk equity universe generation tooling
• Removed dynamic trade pair registry (HL_DYNAMIC_REGISTRY)
• Fixed corporate actions data fetching for dotted ticker symbols (e.g., BRK.B)
• Enabled Polygon WebSocket for equities
• Fixed drawdown threshold calculations for eliminated miners

✅ Strengths

1. Good separation of concerns: Moving trade pair definitions to trade_pair.py improves maintainability
2. Comprehensive testing: Added test_equity_universe.py with solid coverage of edge cases
3. Well-documented generator: generate_equity_universe.py has excellent docstrings explaining the source-of-truth model
4. Backward compatibility: Re-exports from vali_config.py maintain existing API
5. Defensive coding: Databento corporate actions fix handles dotted symbols gracefully with fallback

⚠️ Concerns

Critical Issues

1. Missing Trade Pair File (BLOCKER)

   # vali_objects/trade_pair.py line ~1521
   # File is truncated in the diff!

   The new trade_pair.py file appears cut off mid-definition. The PR cannot be merged without the complete file. Please verify:

   • All Russell 1000 members are properly defined
   • The file ends with proper class methods/properties
   • No syntax errors exist

2. Circular Import Risk

   # vali_objects/enums/miner_asset_class_enum.py
   from vali_objects.trade_pair import TradePair, TradePairCategory, TradePairSource

   Now imports from trade_pair while trade_pair might import from enums. Verify no circular dependency exists. Consider making TradePairCategory a standalone primitives module.

3. Polygon WebSocket for Equities - Performance Concern

   # data_generator/polygon_data_service.py:257
   enabled_websocket_categories={TradePairCategory.CRYPTO, TradePairCategory.FOREX, TradePairCategory.EQUITIES}

   Adding 1000+ equity symbols to WebSocket subscriptions could cause:

   • Message rate throttling from Polygon
   • Memory pressure from buffering 1000+ streams
   • Connection instability

   Recommendation: Add subscription batching/chunking and monitor connection health metrics.

4. Hardcoded Default Values in Generator

   # runnable/generate_equity_universe.py:122
   def default_fees_base(config_text: str) -> tuple[str, str]:
       """Default (fees, base) for new tickers — read from an existing equity-spot member..."""

   While the docstring says "never hardcoded", if the regex fails (no reference member exists), the function raises without fallback. Add explicit defaults or better error messaging.

Major Issues

5. Eliminated Miner Threshold Logic - Potential IndexError

   # vali_objects/challenge_period/challengeperiod_manager.py:164-170
   if self.current_bucket == MinerBucket.ELIMINATED:
       if len(self.entries) >= 2:
           prev_entry = self.entries[-2]
           return prev_entry.bucket.intraday_drawdown_threshold(prev_entry.start_time_ms)
       else:
           return MinerBucket.SUBACCOUNT_CHALLENGE.intraday_drawdown_threshold()

   Good defensive fix, but the fallback to SUBACCOUNT_CHALLENGE threshold is arbitrary. Consider:

   • Documenting why this specific bucket's threshold is appropriate
   • Whether eliminated miners should even have thresholds checked (early return?)

6. Dual-Class Ticker Mapping Maintenance Burden

   # runnable/generate_equity_universe.py:61-67
   IWB_TO_CANONICAL = {
       "BRKB": "BRK_B",
       "BFA": "BF_A",
       # ...
   }

   Manual mapping that requires updates when Russell adds new dual-class stocks. The comment notes this, but consider:

   • Add validation that flags unmapped dotted tickers in CI
   • Auto-detect pattern like ([A-Z]+)([AB])$ -> $1_$2

7. Data Source Priority Change - Breaking?

   # vali_objects/price_fetcher/live_price_fetcher.py:249
   return poly_sources + t_sources + hl_sources + databento_sources

   Previously equities used only Databento (return databento...). Now uses all sources with Databento last. This changes price source precedence—is this intentional? Could affect position valuations.

Moderate Issues

8. Inconsistent Blocklist Access Pattern

   # Multiple files now import BLOCKED_TRADE_PAIR_IDS from trade_pair.py
   # but vali_config.py used ValiConfig.BLOCKED_TRADE_PAIR_IDS

   Mixed access patterns across codebase. Standardize on one:

   • Either from vali_objects.trade_pair import BLOCKED_TRADE_PAIR_IDS
   • Or keep in ValiConfig for consistent namespace

9. HL Dynamic Registry Removal - Migration Path?

   # Removed HL_DYNAMIC_REGISTRY, HL_COIN_TO_DYNAMIC_TRADE_PAIR

   If production has positions tied to dynamic pairs, this could break lookups. Verify:

   • No open positions reference dynamic trade pair IDs
   • Backward compatibility for historical position data
   • Migration script if needed

10. CSV Not Committed - Reproducibility

    # .gitignore:199
    +# Raw iShares Russell 1000 holdings export
    +IWB_holdings.csv

    Raw file gitignored (good), but clean CSV is committed (runnable/equity_universe/russell1000.csv). Ensure team knows to run generator after pulling to sync with any manual edits to trade_pair.py.

💡 Suggestions

1. Add Trade Pair Validation Tests

   # Suggested: tests/vali_tests/test_trade_pair.py
   def test_all_trade_pairs_have_required_fields():
       for tp in TradePair:
           assert tp.fees > 0
           assert tp.min_leverage < tp.max_leverage
           assert tp.subaccount_tier_base_leverage.value > 0

2. Generator: Add Dry-Run Mode

   # runnable/generate_equity_universe.py
   ap.add_argument("--dry-run", action="store_true", 
                   help="Show what would be added without modifying files")

3. Databento Symbol Mapping - Cache It

   # data_generator/databento_data_service.py:369
   query_map = {s.replace(".", ""): s for s in symbols}

   Consider caching this mapping if get_corporate_actions is called frequently (hot path check needed).

4. Equities Min Leverage Mismatch

   # trade_pair.py:18
   EQUITIES_MIN_LEVERAGE = 0.01
   # vs trade_pair.py example members use 0.00009 fees

   Verify 0.01 min leverage is intentional (seems high for equities). Document the rationale or adjust if copy-paste error.

5. Type Hints for Generated Members
   The generator creates members as lists. Consider using a dataclass or NamedTuple for better IDE support:

   class TradePairValue(NamedTuple):
       trade_pair_id: str
       trade_pair: str
       fees: float
       # ...

6. Performance: Lazy Load Trade Pairs
   With 1000+ members, module import time may increase. Profile and consider lazy initialization if it becomes an issue.

🔒 Security Notes

1. CSV Injection Risk - Low

   # runnable/generate_equity_universe.py:146
   w.writerow([r["symbol"], r["name"], r["sector"]])

   If IWB CSV contains malicious formulas (e.g., =CMD|'/C calc'!A1), they could execute in Excel. Sanitize or prefix with ' for safety.

2. Input Validation Missing

   # generate_equity_universe.py:134
   symbol = IWB_TO_CANONICAL.get(ticker, ticker)

   No validation that symbol doesn't contain Python keywords or reserved names. Add validation:

   import keyword
   if keyword.iskeyword(symbol) or symbol.startswith('_'):
       raise ValueError(f"Invalid symbol: {symbol}")

3. File Write Without Backup

   # generate_equity_universe.py:203
   with open(CONFIG_PATH, "w") as f:
       f.write(new_text)

   Direct overwrite of critical config file. Consider:

   • Create backup before writing: shutil.copy2(CONFIG_PATH, f"{CONFIG_PATH}.bak")
   • Atomic write (write to temp, then rename)

4. No Authentication for Data Sources
   Adding 1000 equities increases API call volume. Verify rate limits won't expose API keys through error messages in logs.

📋 Additional Recommendations

1. PR Description: Template checklist is empty. Please fill in:

   • Description of changes
   • Related JIRA tickets
   • Testing confirmation
   • Breaking changes disclosure (HL dynamic registry removal)

2. Documentation: Update relevant docs:

   • How to add new equity symbols
   • Reconstitution schedule (semi-annual from 2026)
   • Data source priority for equities

3. Monitoring: Add metrics for:

   • Number of active trade pairs
   • WebSocket subscription count
   • Price fetch latency per category

4. Gradual Rollout: Consider feature flag for equities trading given scale of change

---

Verdict: Significant refactoring with good intentions but REQUIRES COMPLETION (trade_pair.py file truncated). Address critical issues before merge, especially Polygon WebSocket scalability testing with 1000+ streams. The architecture improvements are sound, but execution details need attention.