Update forex slippage values, increase pnl weight

[sli-tao]

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 updates forex slippage calculations and significantly rebalances scoring weights, shifting emphasis from diversified metrics (50% PnL + 50% risk-adjusted ratios) to PnL-dominant scoring (90% PnL + 10% risk-adjusted ratios). It also introduces time-based forex slippage differentiation and fixes a bug in PnL aggregation across subcategories.

---

✅ Strengths

1. Bug Fix Identified: The addition of pnl_gain and pnl_loss aggregation in asset_segmentation.py (lines 113-114) appears to fix a legitimate bug where these fields weren't being summed during checkpoint aggregation.

2. Test Coverage: Good test coverage added for the aggregation fix (test_aggregate_pnl_single_subcategory) with explicit verification of aggregated values.

3. Parameterization: Making weighting_distribution() and average() methods accept configurable decay parameters improves flexibility and testability.

4. Time-Zone Awareness: Using ZoneInfo for timezone handling in slippage calculations is the modern, correct approach.

---

⚠️ Concerns

CRITICAL: Scoring Weight Changes

Impact Analysis Required - vali_config.py lines 241-247:

# Changed from balanced approach to PnL-dominant
SCORING_PNL_WEIGHT = 0.9  # Was 0.5
# All other metrics reduced from 0.1 to 0.02

This is a fundamental change to the incentive structure:

• Reduces emphasis on risk-adjusted returns by 80%
• Makes system vulnerable to high-risk, high-variance strategies
• Miners can now ignore Sharpe, Sortino, Omega, Calmar ratios almost entirely
• Could encourage reckless trading behavior since risk metrics now matter 5x less

Questions:

1. Has this been validated through backtesting with historical miner data?
2. What's the risk of encouraging overleveraged positions?
3. How does this align with the stated goal of "long-term value of strategies" mentioned in the docs?

CRITICAL: Incomplete PR Template

The PR description has empty sections:

• No description of changes provided
• No related JIRA issues
• No reviewer instructions
• All checkboxes unchecked

This makes it difficult to understand the business context and rationale.

MAJOR: Forex Slippage Model Change

price_slippage_model.py lines 116-126:

if 17 <= hour < 18:  # Daily 5-6 pm EST
    return 0.001     # 10 bps
else:
    return 0.0005    # 5 bps

Issues:

1. Magic Numbers: No explanation for why 10 bps vs 5 bps
2. Timezone Edge Cases: What happens during DST transitions?
3. Simplified Model: The previous model used actual spread and volatility data. This flat-rate approach may be less accurate
4. G1 vs Other Distinction Removed: The previous V2 logic had different rates for G1 (10 bps) vs others (15 bps). Now all forex pairs are treated identically except for time of day

MODERATE: Aggressive PnL Weighting

vali_config.py line 173:

WEIGHTED_AVERAGE_DECAY_MIN_PNL = 0.045  # 70% weight in first 30 days

Combined with the documentation change showing:

• First 10 days: 40% of total score
• First 30 days: 70% of total score
• First 70 days: 87% of total score

This creates extreme recency bias:

• A bad week can destroy months of good performance
• Discourages long-term strategic thinking
• May cause miners to game the system with short-term tactics

MINOR: Test Precision

test_metrics.py line 685:

self.assertAlmostEqual(weighted_score, 70.0, delta=1.0, ...)

A delta of 1.0 (±1.43% tolerance) seems loose for validating a specific weighting distribution. Consider tightening to delta=0.5.

---

💡 Suggestions

1. Provide Business Context

Add to PR description:

• Why the dramatic shift to PnL-focused scoring?
• What problem does this solve?
• What simulations/analyses support this change?

2. Staged Rollout

Consider a more gradual transition:

# Phase 1: Moderate shift
SCORING_PNL_WEIGHT = 0.65  # Instead of jumping to 0.9
SCORING_SHARPE_WEIGHT = 0.07  # Instead of dropping to 0.02

3. Add Constants for Forex Slippage

class ValiConfig:
    FOREX_SLIPPAGE_HIGH_VOLATILITY_HOUR_START = 17
    FOREX_SLIPPAGE_HIGH_VOLATILITY_HOUR_END = 18
    FOREX_SLIPPAGE_HIGH_BPS = 0.001
    FOREX_SLIPPAGE_LOW_BPS = 0.0005

4. Document Slippage Rationale

Add a comment explaining why 5-6 PM EST has higher slippage:

# Higher slippage during 5-6 PM EST due to:
# - London market close (5 PM GMT = 12 PM EST)
# - NYSE close (4 PM EST)
# - Reduced liquidity during transition periods

5. Add Validation for Weight Sum

# In vali_config.py or a test
assert abs(sum([
    SCORING_OMEGA_WEIGHT,
    SCORING_SHARPE_WEIGHT,
    SCORING_SORTINO_WEIGHT,
    SCORING_STATISTICAL_CONFIDENCE_WEIGHT,
    SCORING_CALMAR_WEIGHT,
    SCORING_RETURN_WEIGHT,
    SCORING_PNL_WEIGHT
]) - 1.0) < 0.001, "Scoring weights must sum to 1.0"

6. Add Integration Test

Test that the new weighting actually produces the documented distribution:

def test_pnl_weight_distribution_matches_docs():
    """Verify that 10 days = 40%, 30 days = 70%, 70 days = 87%"""
    # Create strategic test data and verify weights

7. Version Documentation

Add a changelog entry explaining V2 slippage model changes and the rationale.

---

🔒 Security Notes

1. No Direct Security Vulnerabilities: The code changes don't introduce obvious security flaws.

2. Economic Attack Surface: The extreme PnL weighting could enable economic attacks:

   • Miners might exploit short-term volatility
   • Wash trading detection becomes more critical
   • Front-running concerns if PnL is measured too frequently

3. Timezone Dependency: The ZoneInfo import requires the system to have timezone data installed. Consider adding error handling:

   try:
       eastern_tz = ZoneInfo("America/New_York")
   except ZoneInfoNotFoundError:
       # Fallback or raise clear error

---

🎯 Action Items

Before Merging:

1. ✅ MUST: Fill out PR template completely with business justification
2. ✅ MUST: Provide backtesting results showing impact of weight changes
3. ✅ MUST: Add constants for magic numbers in forex slippage
4. ⚠️ SHOULD: Consider staged rollout of weight changes
5. ⚠️ SHOULD: Add validation that scoring weights sum to 1.0
6. ⚠️ SHOULD: Document slippage model V2 rationale in code comments
7. 💡 CONSIDER: Tighten test precision tolerance

Risk Assessment: ⚠️ MEDIUM-HIGH - The scoring weight changes fundamentally alter system incentives and could have unintended consequences. Recommend thorough validation before deployment.