Skip to content

fix(src): Fix 24h price gap calculation by widening retention and lookup window#300

Merged
aristidesstaffieri merged 5 commits intomainfrom
fix/24h-price-gaps
Apr 2, 2026
Merged

fix(src): Fix 24h price gap calculation by widening retention and lookup window#300
aristidesstaffieri merged 5 commits intomainfrom
fix/24h-price-gaps

Conversation

@aristidesstaffieri
Copy link
Copy Markdown
Contributor

@aristidesstaffieri aristidesstaffieri commented Apr 1, 2026

  • Identify actionable new comment (ID 3024455743): update doc comment for threshold constant and use 30 * 60 * 1000 instead of magic number 1800000
  • RETENTION_PERIOD comment already updated to "25 hours" in remote branch
  • Update PRICE_LOOKUP_TOLERANCE_MS value from 1800000 to 30 * 60 * 1000 and improve doc comment
  • Run tests to validate (24/24 pass)

  The Redis Time Series retention period (24h) and the 24h-ago lookup
  threshold (5min tolerance) created only a 5-minute overlap window for
  finding historical price data. As the token count grows and update
  cycles take longer, this narrow window frequently contains no data
  points, causing percentagePriceChange24h to return null.

  - Increase RETENTION_PERIOD from 24h to 25h
  - Increase DEFAULT_ONE_DAY_THRESHOLD_MS from 5min (300000ms) to 30min (1800000ms)

  This expands the overlap window from 5 minutes to 90 minutes, tolerating
  longer update cycles and transient Horizon outages without losing 24h
  price change data.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to reduce null 24h price-change results by increasing the Redis TimeSeries retention period and widening the time window used to find the “~24h ago” historical price point.

Changes:

  • Increased Redis TimeSeries retention from 24h to 25h.
  • Increased the default “one day threshold” delta from 5 minutes to 30 minutes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@piyalbasu piyalbasu left a comment

Choose a reason for hiding this comment

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

This change makes sense to me. The Copilot comments are reasonable to add (they're mostly around documentation and updating tests)

Wondering if this new window size is based on a metric or if this is more of a "let's make it a little bit bigger and see if it works"

  The 24h price change calculation frequently returns null because a single
  threshold (oneDayThreshold) was used for both the minimum history
  requirement and the revRange lookup window. With a 5-minute tolerance and
  24h retention, the overlap window for finding a historical price point was
  only 5 minutes — easily missed during update gaps.

  - Increase RETENTION_PERIOD from 24h to 25h for wider data retention
  - Add separate PRICE_LOOKUP_TOLERANCE_MS (30min) for the revRange lookup
  - Keep DEFAULT_ONE_DAY_THRESHOLD_MS (5min) for the strict ~24h history gate

  The history gate still requires ~23h55m of data before computing a 24h
  change (no stale data). The lookup now searches with a 90-minute overlap
  window instead of 5 minutes, tolerating gaps in price updates., wiring up the handoff from setup to live ingestion
Copy link
Copy Markdown
Contributor

@piyalbasu piyalbasu left a comment

Choose a reason for hiding this comment

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

@aristidesstaffieri mentioned in Slack: I spot checked a few and saw that when they miss their window it wasn't by much, 62 seconds in one case. So this should give it enough wiggle room to limp along for now.

Given that, this PR lgtm!

@aristidesstaffieri
Copy link
Copy Markdown
Contributor Author

This change makes sense to me. The Copilot comments are reasonable to add (they're mostly around documentation and updating tests)

Wondering if this new window size is based on a metric or if this is more of a "let's make it a little bit bigger and see if it works"

I spot checked a few and saw that when they miss their window it wasn't by much, 62 seconds in one case. It varies case by case so I tried to find a value that gave it enough wiggle room to work around the variance without going overboard.

  Add tests verifying that the 24h history gate and the lookup tolerance
  operate independently:

  - Token with 23h54m of history (1min under gate) returns null for
    percentagePriceChange24h and does not attempt the revRange lookup
  - Token with 23h56m of history (1min over gate) computes the 24h change
    using the wider 30min PRICE_LOOKUP_TOLERANCE_MS, not the strict 5min
    DEFAULT_ONE_DAY_THRESHOLD_MS
@aristidesstaffieri aristidesstaffieri merged commit 69c8ac1 into main Apr 2, 2026
6 checks passed
@aristidesstaffieri aristidesstaffieri deleted the fix/24h-price-gaps branch April 2, 2026 15:13
aristidesstaffieri added a commit that referenced this pull request Apr 8, 2026
…300 follow-up)

  Prevents SSRF by ignoring user-provided network_url in /simulate-tx and
  /submit-tx, resolving the RPC server from network_passphrase instead.
  The schema still accepts network_url for backwards compatibility but the
  value is no longer read. Adds tests for both routes covering the new
  behavior.
aristidesstaffieri added a commit that referenced this pull request Apr 8, 2026
…300 follow-up)

  Prevents SSRF by ignoring user-provided network_url in /simulate-tx and
  /submit-tx, resolving the RPC server from network_passphrase instead.
  The schema still accepts network_url for backwards compatibility but the
  value is no longer read. Adds tests for both routes covering the new
  behavior.
aristidesstaffieri added a commit that referenced this pull request Apr 8, 2026
…303)

* rc): remove network_url usage from simulate-tx and submit-tx routes (#300 follow-up)

  Prevents SSRF by ignoring user-provided network_url in /simulate-tx and
  /submit-tx, resolving the RPC server from network_passphrase instead.
  The schema still accepts network_url for backwards compatibility but the
  value is no longer read. Adds tests for both routes covering the new
  behavior.

* chore: remove redundant network_url schema properties and tests

  AJV's removeAdditional:true already silently strips unknown fields,
  so explicitly listing network_url in the schema was unnecessary.
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.

4 participants