Real position sizing

[sli-tao]

Taoshi Pull Request

Description

• Add notional value and quantity attributes to order
• Add realized pnl and unrealized pnl to position
• Add quote->usd and usd->base conversions for cross pair returns
• Migrate existing historical positions and orders, populate usd conversions and sizes
• Update pnl returns calculation
• Update debt ledger pnl scoring

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 implements a real position sizing system by migrating from leverage-only orders to support three sizing methods: leverage, notional value (USD), and quantity (base units). It adds realized/unrealized PnL tracking, USD conversion rates for cross-pair returns, and updates the scoring system accordingly. The changes include a comprehensive migration script to backfill historical data.

---

✅ Strengths

1. Comprehensive Migration Strategy: The migrate_positions_to_quantity_system.py script includes parallel processing, dry-run mode, and detailed progress reporting
2. Backward Compatibility: Maintains leverage-based sizing while adding new options
3. Clear Documentation: Sample signal request updated with helpful comments showing the three sizing options
4. Separation of Concerns: PnL tracking split into realized vs unrealized components makes accounting clearer
5. Robust Error Handling: Migration script includes extensive error handling and reporting

---

⚠️ Concerns

CRITICAL Issues

1. Missing Input Validation (validator.py:1094-1119)

   def parse_order_size(signal, usd_base_conversion, trade_pair, portfolio_value):
       leverage = signal.get("leverage")
       value = signal.get("value")
       quantity = signal.get("quantity")
       
       fields_set = [x is not None for x in (leverage, value, quantity)]
       if sum(fields_set) != 1:
           raise ValueError("Exactly one of 'leverage', 'value', or 'quantity' must be set")

   • No validation that values are positive numbers
   • Division by zero risk if portfolio_value is 0
   • No bounds checking (e.g., quantity could be astronomically large)

2. Race Condition Risk (validator.py:1169-1178)

   usd_base_price = self.live_price_fetcher.get_usd_base_conversion(...)
   quantity, leverage, value = self.parse_order_size(signal, usd_base_price, trade_pair, existing_position.account_size)
   
   order = self._add_order_to_existing_position(existing_position, trade_pair, signal_order_type,
                                               quantity, leverage, value, now_ms, miner_hotkey,
                                               price_sources, miner_order_uuid, miner_repo_version,
                                               OrderSource.ORGANIC, usd_base_price)

   • Price can change between fetching usd_base_price and adding the order
   • Could lead to incorrect position sizing in volatile markets

3. Account Size Fallback Logic (validator.py:1048-1056)

   if existing_position.account_size <= 0:
       bt.logging.warning(
           f"Invalid account_size {existing_position.account_size} for position {existing_position.position_uuid}. "
           f"Using MIN_CAPITAL as fallback."
       )
       existing_position.account_size = ValiConfig.MIN_CAPITAL

   • Modifies state with only a warning - should this be an error?
   • Could mask serious data corruption issues

4. Data Consistency Issue (migrate_positions_to_quantity_system.py:91-102)

   def migrate_order_quantities(position: Position, price_fetcher) -> tuple[int, int]:
       # ...
       if order.price == 0 and order.src == 1: # SKIP elimination order where price == 0
           continue

   • Magic number src == 1 is not explained
   • Silently skipping orders could cause PnL calculation errors

HIGH Priority Issues

5. Missing Transaction Boundaries: The migration script processes positions individually without transaction-like guarantees. If it crashes mid-migration, the data will be in an inconsistent state.

6. Memory Concerns (migrate_positions_to_quantity_system.py:164-205)

   def load_all_positions() -> dict[str, list[Position]]:
       all_positions = defaultdict(list)
       # Loads ALL positions into memory

   • Loading all positions for all hotkeys into memory could cause OOM on large datasets
   • No pagination or streaming

7. Breaking Change Without Version Check: The PR changes fundamental data structures (Order, Position) but doesn't include version checking in the validator to reject old-format signals during transition period.

---

💡 Suggestions

Architecture & Design

1. Add Input Validation Decorator:

   @validate_order_inputs
   def parse_order_size(signal, usd_base_conversion, trade_pair, portfolio_value):
       # Validation happens before function executes

2. Implement Atomic Migration: Consider using a staging area pattern:

   # 1. Copy position to staging
   # 2. Migrate in staging
   # 3. Validate migrated position
   # 4. Atomic swap (rename files)
   # 5. Delete original

3. Add Migration State Tracking: Create a migration status file to track which positions have been migrated:

   migration_state = {
       "version": "7.2.0",
       "started_at": timestamp,
       "completed_hotkeys": [...],
       "failed_hotkeys": [...],
   }

Code Quality

4. Extract Magic Numbers to Constants (migrate_positions_to_quantity_system.py:91):

   ORDER_SOURCE_ELIMINATION = 1
   if order.price == 0 and order.src == ORDER_SOURCE_ELIMINATION:

5. Improve Error Messages:

   # Current
   raise ValueError("Exactly one of 'leverage', 'value', or 'quantity' must be set")
   
   # Better
   raise ValueError(
       f"Exactly one of 'leverage', 'value', or 'quantity' must be set. "
       f"Got: leverage={leverage}, value={value}, quantity={quantity}"
   )

6. Add Type Hints to Migration Script: The migration script is missing type hints which would improve maintainability.

Testing

7. Missing Test Cases:

   • What happens when parse_order_size() receives negative values?
   • What happens when usd_base_conversion is zero?
   • Edge case: extremely small quantities (dust amounts)
   • Edge case: quantities that would exceed exchange limits

8. Add Integration Test: Create a test that:

   • Creates old-format positions
   • Runs migration
   • Validates new format
   • Ensures PnL calculations are identical

9. Test Data Validation (test files): Many test files updated but no new tests added for the new sizing methods.

Performance

10. Optimize Migration Script Memory Usage:

    def load_positions_streaming(hotkey: str):
        """Generator that yields positions one at a time"""
        for trade_pair in TradePair:
            for position_file in iter_position_files(hotkey, trade_pair):
                yield load_position(position_file)

11. Add Progress Persistence: Save migration progress every N positions so crashes don't require full restart.

12. Consider Batching File Writes: Instead of writing each position immediately, batch writes could improve I/O performance.

---

🔒 Security Notes

1. Division by Zero Risk (validator.py:1109, 1114):

   quantity = (value * usd_base_conversion) / trade_pair.lot_size  # lot_size could be 0
   leverage = value / portfolio_value  # portfolio_value could be 0

   Recommendation: Add explicit zero checks with appropriate error handling.

2. Price Manipulation Risk: If get_usd_base_conversion() can be influenced by external price feeds, attackers could manipulate position sizing by timing orders during price feed updates.

3. Integer Overflow Potential: When calculating quantity * trade_pair.lot_size, large quantities could overflow. Use Decimal for financial calculations.

4. Migration Script Security:

   • Runs with file system write access
   • No checksum/signature verification of migration logic
   • Could potentially corrupt all historical data if compromised
     Recommendation: Add integrity checks and limit blast radius.

5. API Key Exposure (sample_signal_request.py:46):

   'api_key': 'xxxx'

   Good that it's redacted, but ensure documentation emphasizes secure API key handling.

---

🐛 Specific Issues

File: neurons/validator.py

• Line 1051: The fallback to MIN_CAPITAL should probably fail the order instead of silently continuing with incorrect data
• Line 1162-1178: Consider extracting this complex order creation logic into a separate method for better testability

File: runnable/migrate_positions_to_quantity_system.py

• Line 91: Should use an enum instead of magic number for order source
• Line 95: Should the migration continue if USD rates fail? Currently just logs warning
• Line 164: load_all_positions() could run out of memory - needs streaming approach
• Line 447: No rollback mechanism if migration fails partway through

File: mining/run_receive_signals_server.py

• Lines 61-63: The ternary operators for None checking are good, but consider using dataclasses.field(default=None) or Pydantic for cleaner validation

File: template/protocol.py

• Lines 53, 60: Copy-paste error - should be CollateralRecord and AssetSelection, not GetDashData ✅ (This is actually fixed in the PR)

---

📋 Additional Observations

1. Migration Timeline: No clear guidance on when validators should run the migration or if it's automatic

2. Backward Compatibility Window: How long will the old leverage-only format be supported?

3. Monitoring: Should add metrics to track:

   • Number of positions migrated
   • Migration success/failure rate
   • Time taken for migration
   • Distribution of sizing methods used by miners

4. Documentation Gap: The PR description mentions updating documentation but checkbox is unchecked. The miner.md changes are good but API documentation for the new fields would be helpful.

---

🎯 Recommendations Priority

Must Fix Before Merge:

1. Add input validation to parse_order_size() (bounds, types, zero checks)
2. Fix division-by-zero risks
3. Add migration state tracking/recovery mechanism
4. Add integration tests for new sizing methods

Should Fix:
5. Implement streaming for migration to avoid OOM
6. Add rollback capability to migration
7. Extract magic numbers to constants
8. Improve error messages

Nice to Have:
9. Add performance metrics/monitoring
10. Document migration procedure for validators
11. Add backward compatibility tests

---

Final Verdict

This is a substantial improvement to the position sizing system with well-thought-out migration strategy. However, there are critical security and data integrity concerns that must be addressed before production deployment. The core logic is sound, but defensive programming (validation, error handling, atomicity) needs strengthening.

Recommendation: Request changes for critical issues, particularly around input validation, division-by-zero handling, and migration atomicity.