Correctly call live/historic prices for rest

[rlarkdfus]

Taoshi Pull Request

Description

Pass order time for historic price calls / pass None for live price calls to data services.
Order times are unified to one timestamp for each trade pair.

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

[deepsource-io]

Here's the code health analysis summary for commits bb84728..02765d7. View details on DeepSource ↗.

Analysis Summary

Analyzer	Status	Summary	Link
Shell	✅ Success		View Check ↗
Python	❌ Failure	❗ 28 occurences introduced 🎯 31 occurences resolved	View Check ↗
Test coverage	❌ Failure	❗ 8 occurences introduced 🎯 7 occurences resolved	View Check ↗

Code Coverage Report

Metric	Aggregate	Python
Branch Coverage	100%	100%
Composite Coverage	78.7% (up 0.1% from main)	78.7% (up 0.1% from main)
Line Coverage	78.7% (up 0.1% from main)	78.7% (up 0.1% from main)
New Branch Coverage	100%	100%
New Composite Coverage	28.2%	28.2%
New Line Coverage	28.2%	28.2%

---

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

[jbonilla-tao]

nice cleanup!

[jbonilla-tao]

Nice fix. this could have caused errors

[jbonilla-tao]

why not pass the time from before? It is the closest timestamp to the order received time as possible.

[rlarkdfus]

The data services check whether or not there is any value assigned to time_ms, so even if the timestamp is right now, it will try to retrieve historical data. It looked like the validator was trying to fetch live data, so to call the same endpoint live endpoint it called before, I passed in None.

[jbonilla-tao]

The goal of this code segment is to get the price closest to the price when the order was received by the validator. That's why the code passed "now_ms" to get_sorted_price_sources_for_trade_pair. By using the current time, it will be a little bit in the future compared to when the order was placed.

[sli-tao]

unsure what this comment means

[sli-tao]

are these changes necessary or is it only for the local testing

[github-actions]

🤖 Claude AI Code Review

Last reviewed on: 14:23:47

Summary

This PR unifies timestamp handling for historic vs. live price calls across data services. The main changes involve:

• Replacing trade-pair-specific timestamp maps with a single unified time_ms parameter
• Introducing a live boolean flag to distinguish between live and historic price fetching
• Refactoring REST endpoints to accept time_ms and use it consistently
• Simplifying the crypto price fetching logic in Tiingo by consolidating two different endpoints into one

✅ Strengths

• Cleaner API: Unified timestamp approach simplifies the interface significantly
• Good refactoring scope: The changes are focused on a specific issue without unnecessary scope creep
• Practical approach: Using a boolean live flag is straightforward and understandable
• Thread safety maintained: Parallel fetching patterns remain intact

⚠️ Concerns

Critical Issues

1. Breaking API Change Without Clear Migration Path (Lines 644-654 in tiingo_data_service.py)

   def get_close_rest(self, trade_pair: TradePair, timestamp_ms: int, live=True)

   The signature changed from attempting_prev_close and optional target_time_ms to required timestamp_ms. This will break existing callers not updated in this PR.

2. Inconsistent NULL Handling (Multiple locations)

   # vali_dataclasses/price_source.py:69
   if not now_ms:
       now_ms = TimeUtil.now_in_millis()

   vs.

   # live_price_fetcher.py:102
   if not time_ms:
       time_ms = TimeUtil.now_in_millis()

   The live_price_fetcher.py:102 should use explicit None check (if time_ms is None) since 0 is a valid timestamp.

3. Incomplete Error Handling (Line 680, tiingo_data_service.py)

   ps = tds.get_close_rest(TradePair.TAOUSD, timestamp_ms=None, live=True)

   Passing None for timestamp_ms when it's typed as int (not Optional[int]) will cause runtime issues.

4. Test Mock Signature Mismatch (Lines 63-67, mock_classes.py)

   def get_sorted_price_sources_for_trade_pair(self, trade_pair, time_ms=None, live=True):
       return [PriceSource(open=1, high=1, close=1, low=1, bid=1, ask=1)]

   The mock now accepts different parameters but doesn't validate them. Tests may pass while production code fails.

Moderate Issues

5. Logic Inversion Confusion (Lines 369, 435, 459, 528 in tiingo_data_service.py)

   if not live:  # Actually means "is historic"

   This is readable but inverting boolean logic can be confusing. Consider is_historic or use_historic_data instead.

6. Removed Fallback Logic Without Explanation (Lines 523-640, tiingo_data_service.py)
   The crypto endpoint completely changed from using top endpoint for live data to always using the prices endpoint. This is a significant behavior change that needs validation:

   • Does the new endpoint have the same latency?
   • Are there rate limit implications?
   • What happens if the new endpoint doesn't return data for very recent timestamps?

7. Misleading Variable Names (Line 553, tiingo_data_service.py)

   data_time_ms = TimeUtil.parse_iso_to_ms(closest_data["date"]) + timespan_ms

   Adding timespan_ms to align with close price time, but this is not documented and could be confusing.

8. Deprecated Code Not Removed (Line 607)

   # Previously used for deprecated tiingo top-of-book rest endpoint - not used anymore (06/24/2025)
   def get_best_crypto_price_info(self, book_data, now_ms, preferred_exchange):

   This method should be removed if it's no longer used, not just commented as deprecated.

💡 Suggestions

1. Type Safety Improvements

   # Before
   def get_closes_rest(self, trade_pairs: List[TradePair], time_ms, live=True) -> dict:
   
   # After
   def get_closes_rest(
       self, 
       trade_pairs: List[TradePair], 
       time_ms: Optional[int] = None,  # None means "now"
       live: bool = True
   ) -> Dict[TradePair, PriceSource]:

2. Consistent Parameter Naming
   Use either time_ms or timestamp_ms consistently throughout. Currently mixed usage creates confusion.

3. Add Parameter Validation

   def get_closes_rest(self, trade_pairs: List[TradePair], time_ms: Optional[int] = None, live: bool = True):
       if not live and time_ms is None:
           raise ValueError("time_ms must be provided for historic price queries")
       if live and time_ms is None:
           time_ms = TimeUtil.now_in_millis()

4. Document the Behavior Change
   Add docstrings explaining the new parameters:

   """
   Fetch closing prices for trade pairs.
   
   Args:
       trade_pairs: List of trade pairs to fetch
       time_ms: Target timestamp in milliseconds. If None and live=True, uses current time.
       live: If True, fetches latest available price. If False, fetches historic price at time_ms.
       
   Returns:
       Dictionary mapping trade pairs to their price sources
   """

5. Add Logging for Price Source Selection

   if not live:
       bt.logging.debug(f"Fetching historic price for {ticker} at {TimeUtil.millis_to_formatted_date_str(time_ms)}")

6. Refactor Repeated Code (Line 545-575)
   The PriceSource creation logic is similar across forex/crypto/equities. Consider extracting to a helper method.

7. Version Notes Should Include Breaking Changes
   In meta/meta.json, the version bump from 7.1.2 to 7.1.3 should be documented as containing breaking API changes (should probably be 7.2.0 or 8.0.0 depending on your versioning strategy).

🔒 Security Notes

1. No SQL Injection Risk: No database queries modified

2. API Key Handling: Unchanged, still passed through safely

3. Input Validation Missing: time_ms parameter is not validated for reasonable ranges. Consider adding:

   if time_ms is not None:
       min_valid_time = 946684800000  # Jan 1, 2000
       max_valid_time = TimeUtil.now_in_millis() + 86400000  # +1 day
       if not (min_valid_time <= time_ms <= max_valid_time):
           raise ValueError(f"Invalid timestamp: {time_ms}")

4. Thread Safety: The parallel fetching with ThreadPoolExecutor remains safe, no shared state issues introduced

📋 Additional Notes

Testing Concerns:

• No unit tests added despite the checklist item being "not applicable"
• This is a significant API change that should have tests
• The mock objects are updated but don't verify the new behavior
• Recommend adding tests for:
  • Live price fetching with time_ms=None
  • Historic price fetching with specific timestamp
  • Error cases (invalid timestamps, missing data)

Documentation:

• The PR description mentions "Order times are unified to one timestamp for each trade pair" but the implementation shows all trade pairs now use the same timestamp
• This is actually a simplification but should be clearly documented

Deployment Risk:

• This changes core pricing logic
• Recommend staged rollout with monitoring
• The "testnet testing" checkbox is marked but no test results are shared

Recommendation

Conditional Approval - The core approach is sound, but requires fixes before merging:

1. Fix the type annotations and null handling
2. Add validation for the time_ms parameter
3. Remove or properly deprecate unused code
4. Add basic unit tests
5. Update documentation with breaking change notice