Skip to content

fix(hermes): improve TWAP reliability - non-optional price selection and consistent time windows #2521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 26, 2025

Conversation

tejasbadadare
Copy link
Contributor

@tejasbadadare tejasbadadare commented Mar 25, 2025

Summary

This PR refactors how Hermes fetches the data for the TWAP calculation to improve reliability.

Rationale

  • The internal API for TWAP now accepts window_seconds: u64 and end_time: RequestTime, instead of start_time: RequestTime and end_time: RequestTime.
    • This allows us to first fetch the end messages, then calculate the start_time based on the publish time of the end messages. Previously, the actual time difference between start and end messages could differ from the intended window size since the latest timestamp in the cache could be older than the wall time. This change guarantees that the start and end messages are window_seconds apart.
    • This also aligns better with the semantics we want to uphold for the public API.
  • Added a new RequestTime::LatestTimeEarliestSlot request mode to the cache that selects messages with the latest publish time but earliest slot number. This follows the same semantics as FirstAfter, and helps choose non-optional (canonical) prices.
    • Many requests for latest TWAP were returning "Update data not found" because the TWAP calculation will fail if it is given a message where prev_publish_time == publish_time. This is done to avoid optionality of prices, and we consider only the message where prev_publish_time < publish_time as the canonical price for that timestamp. Previously we were using the RequestTime::Latest mode, which just returns the latest message for a feed, NOT the latest canonical message. The new mode provides these semantics.
  • Improved error handling and logging for TWAP calculation failures. Previously, they were not getting logged, and errors were masked by a generic "Update data not found" message, which made investigation difficult.

How has this been tested?

  • Current tests cover my changes
  • Added new tests
    • Added test_get_verified_twaps_with_update_data_uses_non_optional_prices to verify correct message selection
    • Added test_latest_time_earliest_slot_request_works to test the LatestTimeEarliestSlot cache request mode
  • Manually tested the code
    • Tested with local server and Postman, we don't see the high rate of "Update not found" errors anymore, and the timestamps are always window_seconds apart.

Copy link

vercel bot commented Mar 25, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Visit Preview Mar 25, 2025 8:12pm
component-library ⬜️ Ignored (Inspect) Visit Preview Mar 25, 2025 8:12pm
entropy-debugger ⬜️ Ignored (Inspect) Visit Preview Mar 25, 2025 8:12pm
insights ⬜️ Ignored (Inspect) Visit Preview Mar 25, 2025 8:12pm
proposals ⬜️ Ignored (Inspect) Visit Preview Mar 25, 2025 8:12pm
staking ⬜️ Ignored (Inspect) Visit Preview Mar 25, 2025 8:12pm

@tejasbadadare tejasbadadare changed the title refactor(hermes): improve TWAP reliability - non-optional price selection and consistent time windows fix(hermes): improve TWAP reliability - non-optional price selection and consistent time windows Mar 25, 2025
@@ -285,6 +285,28 @@ async fn retrieve_message_state(
Some(key_cache) => {
match request_time {
RequestTime::Latest => key_cache.last_key_value().map(|(_, v)| v).cloned(),
RequestTime::LatestTimeEarliestSlot => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought i had while working on this... should we use this for all our latest endpoints, not just TWAP?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. If we change it to the earliest slot then effectively we'll have 1 price per second. I think the poll users (and subscribers) would still like to get the latest price.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense. I guess they are on the hook for ensuring the price they use is canonical?

Copy link
Collaborator

@ali-behjati ali-behjati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@tejasbadadare tejasbadadare merged commit 337d9c1 into main Mar 26, 2025
9 checks passed
@tejasbadadare tejasbadadare deleted the tb/hermes/fix-twap-earliest-timestamp-in-slot branch March 26, 2025 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants