Follow ups + challenge fixes

[ward-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: 14:23:47

---

Summary

This PR introduces several key changes:

1. Adjusted Tiingo polling interval from 5s to 8.6s
2. Changed scoring weights to 100% Average Daily PnL (removing other metrics)
3. Refactored challenge period scoring to use asset_softmaxed_scores instead of combined_scores_dict
4. Added anti-gaming validation for market orders with SL/TP
5. Enhanced limit order bracket order creation with better validation
6. Removed minimum positions check for challenge period miners with ledgers
7. Added score caching in ChallengePeriodManager for MinerStatisticsManager

---

✅ Strengths

• Comprehensive test coverage: Excellent addition of anti-gaming tests (lines 1585-1810 in test_limit_orders.py)
• Security improvement: Strong validation against gaming mechanics for SL/TP orders
• Better error handling: SignalException properly propagated from bracket order creation
• Code simplification: Refactored scoring logic reduces complexity
• Clear documentation: Well-commented test helper methods and validation logic
• Performance optimization: Caching scores reduces redundant calculations

---

⚠️ Concerns

CRITICAL ISSUES

1. Breaking Change - Scoring Algorithm (docs/miner.md:113-120)

   -| Average Daily PnL      | 90%            |
   +| Average Daily PnL      | 100%           |

   • This is a major breaking change that fundamentally alters how miners are scored
   • All other metrics (Calmar, Sharpe, Omega, Sortino, Statistical Confidence) are now 0%
   • No explanation in PR description for this dramatic shift
   • Could significantly impact miner behavior and system fairness
   • RECOMMENDATION: This needs community notification and potentially a longer testing period

2. Removed Minimum Positions Check (challengeperiod_manager.py:348-352)

   # Minimum positions check not necessary if they have ledger. If they have a ledger, they can be scored.
   # has_minimum_positions, inspection_positions = ChallengePeriodManager.screen_minimum_positions(positions, hotkey)
   # if not has_minimum_positions:
   #     miners_not_enough_positions.append(hotkey)
   #     continue

   • Commented out code should be deleted, not left in
   • Comment logic seems weak - having a ledger doesn't guarantee meaningful trading activity
   • This could allow miners to game the system with minimal positions
   • RECOMMENDATION: Either remove completely or add better justification

3. Magic Number Without Explanation (tiingo_data_service.py:40)

   -POLLING_INTERVAL_S = 5
   +POLLING_INTERVAL_S = 8.6

   • Why 8.6 seconds specifically? This seems arbitrary
   • No comment explaining the reasoning
   • Could this be related to API rate limits?
   • RECOMMENDATION: Add comment explaining the rationale

4. Potential Race Condition (limit_order_manager.py:723-729)

   if order.execution_type == ExecutionType.LIMIT and (order.stop_loss is not None or order.take_profit is not None):
       self.create_sltp_order(miner_hotkey, order)
   
   except SignalException as e:
       error_msg = f"Limit order [{order.order_uuid}] filled successfully, but bracket order creation failed: {e}"
       bt.logging.warning(error_msg)

   • If bracket order creation fails after the limit order fills, the position is left unprotected
   • Only logs a warning but doesn't halt execution
   • RECOMMENDATION: Consider if this should be a harder failure or if there should be retry logic

MAJOR ISSUES

5. Inconsistent Method Naming (limit_order_client.py:286)

   def create_sltp_order(self, miner_hotkey: str, parent_order):

   • Method renamed from create_sltp_orders (plural) to create_sltp_order (singular)
   • All call sites updated, which is good
   • RECOMMENDATION: Verify this isn't a breaking change for external integrations

6. Removed Counter Reset (limit_order_manager.py:1027-1030)

   -def _reset_counters(self):
   -    """Reset evaluation counters."""
   -    self._limit_orders_evaluated = 0
   -    self._limit_orders_filled = 0

   • These counters are referenced elsewhere but no longer reset
   • Could lead to unbounded growth if counters are still incremented
   • RECOMMENDATION: Verify counters are properly removed from entire codebase

7. Test Helper Uses Production Code (test_limit_orders.py:116-143)

   def create_filled_market_order(self, order_type: OrderType = OrderType.LONG, fill_price=50000.0,

   • Helper creates ExecutionType.MARKET orders to bypass validation
   • Comment mentions "avoid limit order validation"
   • This could hide bugs if validation logic changes
   • RECOMMENDATION: Consider if this is the right approach or if we should test the actual flow

8. Flat Order Trigger Logic Changed (test_limit_orders.py:541, 567)

   -self.assertEqual(trigger, 50000.0)
   +self.assertIsNone(trigger)

   • FLAT orders now return None instead of fill price for trigger evaluation
   • No explanation for this behavioral change
   • Could affect how FLAT orders are processed
   • RECOMMENDATION: Add comment explaining why FLAT orders shouldn't have triggers

MODERATE ISSUES

9. Cached Scores Stale Data Risk (challengeperiod_manager.py:117-118)

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

   • Scores are cached but no clear cache invalidation strategy
   • What happens if inspect() is called multiple times?
   • RECOMMENDATION: Add cache invalidation logic or TTL

10. Logging Noise Reduction (limit_order_manager.py:470-472)

    if now_ms - self._last_print_time_ms > 60 * 1000:
        total_orders = sum(len(orders) for hotkey_dict in self._limit_orders.values() for orders in hotkey_dict.values())
        bt.logging.info(f"Checking {total_orders} limit orders across {len(self._limit_orders)} trade pairs")

    • Good: Reduces log spam
    • Concern: 60-second interval is hardcoded
    • Missing initialization of _last_print_time_ms in constructor (added at line 76)
    • RECOMMENDATION: Make interval configurable

11. Incomplete Error Context (limit_order_manager.py:874-876)

    except Exception as e:
        bt.logging.error(f"Error creating bracket order: {e}")
        bt.logging.error(traceback.format_exc())
        raise SignalException(f"Error creating bracket order: {e}")

    • Catches generic Exception which is too broad
    • Should catch specific exceptions
    • RECOMMENDATION: Be more specific with exception types

---

💡 Suggestions

1. Add Configuration Constants

   # tiingo_data_service.py
   # Tiingo WebSocket has rate limit of ~10 requests per minute
   # 8.6s ensures we stay under limit with margin for retries
   POLLING_INTERVAL_S = 8.6

2. Better Validation Error Messages

   # limit_order_manager.py:805
   if parent_order.stop_loss is not None and parent_order.stop_loss >= fill_price:
       raise SignalException(
           f"Invalid LONG bracket order [{parent_order.order_uuid}]: "
           f"stop_loss ({parent_order.stop_loss}) must be < fill_price ({fill_price}). "
           f"This prevents gaming the system by setting SL above entry price."
       )

3. Add Cache Invalidation

   # challengeperiod_manager.py
   def invalidate_score_cache(self):
       """Invalidate cached scores when new inspection runs."""
       self._cached_asset_softmaxed_scores = {}
       self._cached_asset_competitiveness = {}

4. Extract Magic Numbers to Constants

   # limit_order_manager.py
   LOG_INTERVAL_MS = 60 * 1000  # Log summary every 60 seconds
   MINIMUM_FILL_INTERVAL_MS = 1000  # Prevent fills within 1 second

5. Better Test Documentation

   # test_challengeperiod_unit.py:199
   def get_asset_softmaxed_scores(self, miner_scores: dict[str, float], asset_class=None):
       """
       Create asset_softmaxed_scores dict for testing.
       
       This bypasses the normal scoring pipeline and directly provides scores,
       allowing us to test promotion/demotion logic in isolation.
       
       Args:
           miner_scores: dict mapping hotkey to score (0.0 to 1.0)
           asset_class: TradePairCategory, defaults to CRYPTO
       """

6. Consistent Error Handling Pattern

   # order_processor.py:447
   if created_order and (created_order.stop_loss or created_order.take_profit):
       if updated_position and not updated_position.is_closed_position:
           try:
               limit_order_client.create_sltp_order(miner_hotkey, created_order)
           except SignalException as e:
               # Log the error but don't fail the market order
               bt.logging.error(f"Market order succeeded but bracket creation failed: {e}")
               # Optionally: emit metric for monitoring
               # metrics.increment('bracket_order_creation_failure')
               raise

---

🔒 Security Notes

1. Anti-Gaming Validation - EXCELLENT ✅

   • The new validation in create_sltp_order (lines 791-834) effectively prevents miners from:
     • Setting LONG stop loss above entry (guaranteed instant exit)
     • Setting LONG take profit below entry (impossible profit)
     • Setting SHORT stop loss below entry (wrong direction)
     • Setting SHORT take profit above entry (wrong direction)
   • This is a critical security improvement

2. Potential Gaming Vector ⚠️

   • Removing minimum positions check (line 348-352) could allow miners to:
     • Create minimal positions to stay in challenge period
     • Avoid actual trading while appearing active
   • RECOMMENDATION: If this is intentional, document the reasoning clearly

3. Bracket Order Failure Handling ⚠️

   • If bracket order creation fails after successful market order fill (line 446-451):
     • Position is left without SL/TP protection
     • Only raises exception, doesn't attempt recovery
   • RECOMMENDATION: Consider retry logic or automatic position closure on bracket failure

4. Score Manipulation Risk ⚠️

   • Caching scores without proper invalidation (lines 393-394) could lead to:
     • Stale scores being used for critical decisions
     • Potential for score manipulation if cache timing is exploitable
   • RECOMMENDATION: Add explicit cache invalidation and document cache lifecycle

5. Input Validation ✅

   • Good validation of fill_price existence (line 797)
   • Proper order type checking (line 833)
   • Comprehensive edge case handling in tests

---

📋 Checklist Issues

The PR description checklist is incomplete:

• Description is missing (just template text)
• Related Issues (JIRA) not filled in
• No specific reviewer instructions
• Breaking change notification checkbox exists but may not be addressed (scoring change)

---

🎯 Recommendations Priority

MUST FIX BEFORE MERGE:

1. Add explanation for scoring weight change (100% PnL)
2. Resolve commented-out code (delete or implement properly)
3. Add comment explaining 8.6 second polling interval
4. Document or remove minimum positions check bypass
5. Verify counter removal is complete

SHOULD FIX:
6. Add cache invalidation strategy
7. Make magic numbers into named constants
8. Improve error handling specificity
9. Add better inline documentation for complex logic

NICE TO HAVE:
10. Extract test helper methods to shared utilities
11. Add metrics/monitoring for bracket order failures
12. Consider retry logic for bracket order creation

---

Final Assessment

Code Quality: 7/10 - Well-structured but needs documentation improvements
Security: 8/10 - Excellent anti-gaming measures, some cache concerns
Testing: 9/10 - Comprehensive test coverage, especially for edge cases
Performance: 8/10 - Good caching strategy, minor optimization opportunities

Overall: This PR introduces important security improvements but contains a breaking change that needs careful consideration and community notification. The anti-gaming validation is excellent, but the scoring algorithm change (100% PnL weighting) is significant and under-documented.