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: 09:43:27

Summary

This PR refactors the price fetching mechanism to properly distinguish between live and historical price queries. The main changes unify timestamp handling across trade pairs and explicitly pass time_ms and live flags through the data service layer, replacing the previous implicit behavior where None indicated live prices.

✅ Strengths

• Better API clarity: Explicit live boolean parameter is more readable than checking for None
• Unified timestamp handling: Using a single timestamp for all trade pairs in a batch is more consistent
• Consolidation: The crypto endpoint refactoring (lines 527-595 in tiingo_data_service.py) removes duplicate code paths for live vs. historical data
• Thread safety improvements: Parallel fetching with ThreadPoolExecutor remains intact

⚠️ Concerns

CRITICAL: Breaking API Changes

1. Lines 376, 380 (base_data_service.py): Method signatures changed from trade_pair_to_last_order_time_ms dict to single time_ms - this is a breaking change that could cause runtime errors if other code still passes the old parameter format
2. Lines 467-468 (polygon_data_service.py): Removed the if timestamp_ms is None guard without verifying all callers now provide this parameter
3. Line 643 (tiingo_data_service.py): get_close_rest signature completely changed - removed attempting_prev_close parameter that may be used elsewhere

Logic Issues

4. Lines 570-571 (tiingo_data_service.py):

   high=price_close,  # Should this be closest_data['high']?

   Setting high to price_close instead of actual high value seems incorrect

5. Lines 106-107 (live_price_fetcher.py):

   if not time_ms:
       time_ms = TimeUtil.now_in_millis()

   This defeats the purpose of making time_ms required. Either make it optional in the signature or remove this check

6. Line 460 (tiingo_data_service.py): The forex logic inverts the live flag:

   if not live:  # Checking for historical
       attempting_previous_close = not self.is_market_open(tp, time_ms=target_time_ms)

   But this variable is used later without considering the live case

Missing Error Handling

7. Line 546 (tiingo_data_service.py): min(price_data, key=...) will raise ValueError if price_data is empty after the None check
8. Lines 369-370 (tiingo_data_service.py): In get_closes_equities, historic queries throw Exception('TODO') - incomplete implementation

Inconsistencies

9. Lines 434-435 (tiingo_data_service.py): The URL construction for crypto changed from using the "top" endpoint to the "prices" endpoint for live data, which may have different rate limits or response formats
10. Line 335 (tiingo_data_service.py): verbose parameter position inconsistent - it's last in some methods, middle in others

💡 Suggestions

High Priority

1. Add migration path: Consider supporting both old and new signatures temporarily with deprecation warnings

   def get_closes_websocket(self, trade_pairs: List[TradePair], time_ms=None, trade_pair_to_last_order_time_ms=None):
       if trade_pair_to_last_order_time_ms is not None:
           warnings.warn("trade_pair_to_last_order_time_ms is deprecated", DeprecationWarning)
           # Handle old format

2. Fix the high price assignment (line 570):

   high=float(closest_data['high']),

3. Make time_ms truly required - remove fallback checks or make it explicitly Optional[int] with proper None handling

4. Complete the TODO (line 369) for historical equity queries or document why it's not implemented

Medium Priority

5. Add parameter validation:

   def get_closes_rest(self, trade_pairs: List[TradePair], time_ms: int, live=True):
       if time_ms is None or time_ms <= 0:
           raise ValueError("time_ms must be a positive integer")

6. Consolidate the live flag semantics: Some places check if not live, others check if live. Consider using an enum:

   class PriceQueryMode(Enum):
       LIVE = "live"
       HISTORICAL = "historical"

7. Document the endpoint change: Add comments explaining why crypto switched from "top" to "prices" endpoint

8. Standardize parameter order: Put time_ms and live in the same position across all methods

Low Priority

9. Type hints improvement:

   def get_closes_rest(self, trade_pairs: List[TradePair], time_ms: int, live: bool = True) -> Dict[TradePair, PriceSource]:

10. Add docstrings to explain the live parameter behavior:

    """
    Args:
        time_ms: Unix timestamp in milliseconds
        live: If True, fetches current price at time_ms. If False, fetches historical price closest to time_ms
    """

🔒 Security Notes

1. No SQL injection risks: No database queries in this code
2. API key handling: Keys are passed through but not logged - good practice
3. Request timeout: Line 445 has timeout=5 which is good, verify all requests have timeouts
4. Input validation needed: The time_ms parameter should be validated to prevent timestamp manipulation attacks:

   if time_ms > TimeUtil.now_in_millis() + 86400000:  # Max 1 day in future
       raise ValueError("Invalid timestamp")

🧪 Testing Gaps

1. No tests modified: The checklist shows unit tests are not added despite significant API changes
2. Mock class updates (lines 63-67): MockLivePriceFetcher updated but no corresponding test coverage shown
3. Edge cases not covered:
   • What happens when price_data is empty?
   • What if time_ms is far in the past/future?
   • How does this handle market closed scenarios differently now?

📝 Documentation Issues

1. PR description vague: "Order times are unified" - explain WHY this is needed
2. No migration guide: How should existing callers update their code?
3. Incomplete checklist: Documentation and unit tests marked as not done but PR seems ready for review

🏗️ Architecture Observations

Positive:

• Separation of concerns maintained between Polygon and Tiingo services
• Parallel execution strategy preserved

Concerns:

• The live flag creates a dual-mode API which can be confusing. Consider separating into get_live_prices() and get_historical_prices() methods
• The removal of the dictionary parameter makes batching less flexible - you can no longer have different timestamps per trade pair

Recommendation

REQUEST CHANGES - The breaking API changes and incomplete historical implementation need to be addressed before merging. Add proper error handling, complete the TODO items, and include tests.