Equity Returns and Drawdown Calc

[sli-tao]

Taoshi Pull Request

Description

Use account equity to calculate portfolio returns and drawdown.

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 modifies the account size retrieval logic to use real-time timestamps instead of start-of-day calculations, and adds a validation check to prevent withdrawals when miners have open positions. The changes affect both the core business logic and related tests.

✅ Strengths

• Improved timestamp precision: The change from day-based to timestamp-based lookups (line 101 in miner_account_manager.py) provides more granular control and better reflects real-time account states
• Critical business rule enforcement: Added validation preventing withdrawals with open positions (lines 486-490 in validator_contract_manager.py) protects against potential inconsistent states
• Test alignment: Tests were updated to match the new behavior, removing unnecessary day-offset calculations
• Cleaner code: Removed unnecessary date manipulation logic, making the code simpler and easier to reason about

⚠️ Concerns

Critical: Missing Migration Strategy

The change in get_account_size() logic fundamentally alters how historical records are interpreted:

• Old logic: Matched records using valid_date_timestamp (start of day)
• New logic: Matches using update_time_ms (exact timestamp)

Problem: Existing persisted CollateralRecord objects may have update_time_ms values that don't align with the new lookup expectations. This could cause:

1. Historical account sizes to be retrieved incorrectly
2. Edge cases where no matching record is found when one should exist
3. Financial calculation inconsistencies for existing miners

Recommendation: Need a data migration script or backward-compatibility check.

Potential Race Condition (Minor)

In validator_contract_manager.py lines 486-490, the check for open positions and subsequent withdrawal logic are not atomic. Between checking positions and processing withdrawal, new positions could theoretically be opened (depending on your concurrency model).

Recommendation: Consider if this needs transaction-level protection or locking.

Logging Inconsistency

Lines 460 and 548 in miner_account_manager.py now log update_time_ms (a numeric timestamp) instead of valid_date_str (human-readable). This degrades log readability.

# Before: "Updated account size for miner123: $10,000.00 (valid from 2024-01-15)"
# After:  "Updated account size for miner123: $10,000.00 (valid from 1705276800000)"

💡 Suggestions

1. Restore Human-Readable Logging

from datetime import datetime, timezone

# In logging statements (lines 460, 548):
readable_time = datetime.fromtimestamp(collateral_record.update_time_ms / 1000, tz=timezone.utc).isoformat()
bt.logging.info(
    f"Updated account size for {hotkey}: ${account_size:,.2f} (valid from {readable_time})")

2. Add Defensive Check for Edge Cases

In get_account_size() method:

# After line 103
if record.update_time_ms <= timestamp_ms:
    # Add validation
    if record.account_size_theta < 0:
        bt.logging.warning(f"Invalid negative account size for {self.hotkey}, skipping record")
        continue
    theta = min(record.account_size_theta, ValiConfig.MAX_COLLATERAL_BALANCE_THETA)
    return max(theta * ValiConfig.COST_PER_THETA, ValiConfig.MIN_CAPITAL)

3. Enhanced Test Coverage

The current tests don't cover some important scenarios:

def test_account_size_with_future_timestamp(self):
    """Test querying account size with timestamp before any records exist"""
    current_time = int(time.time() * 1000)
    very_old_time = current_time - (365 * self.DAY_MS)  # 1 year ago
    
    self.miner_account_client.set_miner_account_size(self.MINER_1, 0.001, current_time)
    
    # Should return None or MIN_CAPITAL for timestamps before first record
    size = self.miner_account_client.get_miner_account_size(self.MINER_1, very_old_time)
    # Add assertion based on expected behavior

def test_withdrawal_blocked_with_open_positions(self):
    """Test that withdrawal fails when positions are open"""
    # Create open position for miner
    # Attempt withdrawal
    # Assert failure with correct error message

4. Documentation Update Needed

The behavioral change in timestamp handling should be documented:

• Update docstring for get_account_size() method to clarify it now uses exact timestamp matching
• Document the withdrawal restriction in relevant API/user documentation
• Add inline comment explaining the timestamp comparison logic

5. Consider Adding Index/Caching

If collateral_records grows large, the reversed iteration (line 101) could become inefficient. Consider:

# Add to MinerAccount class
def _build_timestamp_index(self):
    """Build sorted index for faster lookups"""
    self._timestamp_index = sorted(
        [(r.update_time_ms, i) for i, r in enumerate(self.collateral_records)]
    )

def get_account_size(self, timestamp_ms=None):
    # Use binary search on _timestamp_index
    import bisect
    # ... implementation

🔒 Security Notes

Medium: Withdrawal Validation Bypass Risk

The new position check (line 486) is good, but consider:

1. Timing attack: Could a miner close positions, trigger withdrawal, then immediately reopen before withdrawal completes?
2. Partial state: What happens if withdrawal succeeds but position closure verification fails?

Recommendation: Implement withdrawal as a two-phase commit or use optimistic locking with version checks.

Low: Integer Overflow in Timestamp Calculations

Using int(time.time() * 1000) for millisecond timestamps is generally safe in Python 3, but be aware:

• Ensure all timestamp comparisons use consistent units (milliseconds throughout)
• Consider adding validation: assert 0 < timestamp_ms < 2**53 (JavaScript MAX_SAFE_INTEGER if this interops with JS)

Low: Missing Input Validation

The withdrawal check doesn't validate miner_hotkey format. Consider adding:

if not miner_hotkey or not isinstance(miner_hotkey, str):
    return {
        "successfully_processed": False,
        "error_message": "Invalid miner hotkey"
    }

🎯 Action Items for PR Author

Must Address:

1. ⚠️ Clarify migration strategy for existing CollateralRecord data
2. ⚠️ Fix logging to display human-readable timestamps
3. ⚠️ Add test case for withdrawal blocking with open positions

Should Consider:
4. Document the behavioral change in method docstrings
5. Add edge case tests (timestamps before first record, etc.)
6. Review concurrency implications of the withdrawal check

Nice to Have:
7. Performance optimization for large collateral_records lists
8. Enhanced input validation

Verdict

CONDITIONAL APPROVE - The core logic improvement is sound, but needs clarification on data migration and logging fixes before merge. The withdrawal validation is a valuable addition. Address the "Must Address" items, and this will be good to go.