Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent scoring/processing when the “realized” (final) candle price is missing, and to improve robustness around when validator requests become eligible for scoring.
Changes:
- Refactors
PriceDataProvider.fetch_data()to unify Pyth vs Hyperliquid fetching and adds a hard check that the final price is notNaN. - Adjusts validator request scoring windowing to wait an extra minute past the request end (to allow Pyth’s last candle to be available).
- Tweaks scheduler exception handling so cycles reset their start time after failures.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
synth/validator/price_data_provider.py |
Refactors fetch flow and introduces a “missing last price” validation. |
synth/validator/miner_data_handler.py |
Changes scoring eligibility timing (+1 minute) and retry/exception behavior in response saving. |
synth/utils/sequential_scheduler.py |
Adjusts exception handling to reset cycle start time and moves first_run flag update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception: | ||
| bt.logging.exception("Error in cycle ") | ||
| cycle_start_time = get_current_time() | ||
| self.first_run = False |
There was a problem hiding this comment.
self.first_run is now set to False even when run_cycle() raises. Previously it was only flipped after a successful cycle; with this change, a failed initial cycle will be treated as a subsequent run, which changes the delay calculation and can slow down recovery/retry scheduling. Consider only setting first_run = False after run_cycle() completes successfully (or track success explicitly).
| except Exception: | |
| bt.logging.exception("Error in cycle ") | |
| cycle_start_time = get_current_time() | |
| self.first_run = False | |
| self.first_run = False | |
| except Exception: | |
| bt.logging.exception("Error in cycle ") | |
| cycle_start_time = get_current_time() |
| if np.isnan(prices[-1]): | ||
| bt.logging.warning( | ||
| f"missing price data for the last timestamp for asset {asset} in request {validator_request.id}" | ||
| ) | ||
| raise ValueError( |
There was a problem hiding this comment.
prices can be empty here (e.g., when _transform_data returns [] for no data, or Hyperliquid returns no candles). Accessing prices[-1] will raise IndexError and break existing behavior/tests (e.g., the no-prices case used to return []). Add an explicit guard before indexing (e.g., return [] early, or raise a clear exception) so missing-data cases don’t crash with IndexError.
| raise ValueError( | ||
| f"missing price data for the last timestamp for asset {asset} in request {validator_request.id}" | ||
| ) |
There was a problem hiding this comment.
This new behavior raises when the last price is NaN, but there’s no unit test covering the "missing last timestamp" path (and the existing "no prices" behavior should remain well-defined). Please add/adjust tests to cover (1) last value missing => error raised, and (2) empty response => returns [] (or whatever behavior is intended).
No description provided.