Skip to content

Revert "Fetch uncached V3 pools on demand so cold quotes find liquidity (#4560)"#4629

Merged
jmg-duarte merged 1 commit into
mainfrom
jmgd/revert-caches
Jul 15, 2026
Merged

Revert "Fetch uncached V3 pools on demand so cold quotes find liquidity (#4560)"#4629
jmg-duarte merged 1 commit into
mainfrom
jmgd/revert-caches

Conversation

@jmg-duarte

Copy link
Copy Markdown
Contributor

Description

The original PR led to fetch_liquidity taking orders of magnitude more than usual on service restarts. When performing tests this becomes particularly bad because of the deployment neighboring nature.

image

Changes

How to test

Tested in prod, notice how the spikes disappear
image

@jmg-duarte
jmg-duarte requested a review from a team as a code owner July 15, 2026 10:19
@jmg-duarte jmg-duarte added the hotfix Labels PRs that should be applied into production right away label Jul 15, 2026
@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jmg-duarte's task in 2m 10s —— View job


PR Review: Revert V3 on-demand pool fetching

  • Gather context (read diff, original commit 8cb2626)
  • Verify the revert is clean and complete
  • Review correctness of reverted code
  • Post findings

Verdict: ✅ Clean revert, no blocking issues

This is a mechanically sound revert of #4560. Key checks:

  • Intermediate commit preserved. Between the original and this revert, Drop the pool-indexer wait-until-timeout config #4561 (2317b58e, drop wait-until-timeout) also touched pool_indexer.rs. The revert correctly keeps that change — it restores the wait_until / get_registered_pools / get_pools_with_ticks_by_ids signatures to u64 while retaining the WAIT_UNTIL_TIMEOUT constant rather than re-introducing the removed wait_until_timeout field. 👍
  • No dangling references. The BlockTarget enum and its resolve_block helper are fully removed; a repo-wide search finds zero remaining references. All call sites (pool_fetching.rs:157/199/279) consistently pass u64 (block_number / fetched_block_number), matching the reverted trait signatures.
  • Behavior restored. PoolsCheckpointHandler::get goes back to returning cache hits and flagging misses into missing_pools for the next maintenance tick; fetch no longer performs on-demand pool fetching — which is exactly the intent, since the on-demand path was what caused fetch_liquidity to spike on restart.
  • get_pools_with_ticks_by_ids empty-ids guard is removed from both sources, restoring the pre-Fetch uncached V3 pools on demand so cold quotes find liquidity #4560 state (no functional regression — this matches the previously-shipped behavior).

The prod graphs in the description confirm the spikes disappear after the revert, which aligns with removing the synchronous on-demand fetch from the quote hot path.

Note

I could not run cargo check / cargo nextest in this environment (command approval not granted), so compilation and the test suite were not executed here. Given the revert is mechanical and the symbol analysis is consistent, I'd expect CI to pass — worth confirming the green check before merging.
jmgd/revert-caches

@AryanGodara AryanGodara left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Looks safe to merge w/o affecting existing running indexers in staging 🫡

@jmg-duarte
jmg-duarte added this pull request to the merge queue Jul 15, 2026
Merged via the queue into main with commit f6b3545 Jul 15, 2026
24 checks passed
@jmg-duarte
jmg-duarte deleted the jmgd/revert-caches branch July 15, 2026 13:34
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 15, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

hotfix Labels PRs that should be applied into production right away

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants