Skip to content

#Closes 220 - #583

Open
actvabdulrahman wants to merge 4 commits into
Divinity-Labs:mainfrom
actvabdulrahman:feat/incremental-pool-discovery
Open

actvabdulrahman wants to merge 4 commits into
Divinity-Labs:mainfrom
actvabdulrahman:feat/incremental-pool-discovery

Conversation

@actvabdulrahman

Copy link
Copy Markdown

Summary

Implements incremental pool discovery per venue so new pools are picked
up without a process restart. Pools that disappear are removed from the
active watched set while retaining their full price history. Added and
removed pool counts are exported as metrics per venue per discovery cycle.

Related issue

Closes #220

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Docs
  • Tests
  • CI / tooling

Checklist

  • I have read CONTRIBUTING.md

  • npx tsc --noEmit passes

  • npm run build passes

  • I added / updated tests where relevant

  • I updated docs where relevant

  • New pools are picked up within a configurable re-discovery interval without a restart

  • Disappeared pools are removed from the active watched set after the next discovery cycle

  • Disappeared pools' price_points history is fully retained

  • pools_added and pools_removed counts exported as metrics per venue per cycle

  • Re-discovery interval is configurable per venue — not hard-coded

  • Discovery cycle test added covering a two-cycle mock: one pool added, one removed, metrics and watched set both asserted

  • Follows aquarius.ts reference structure including _resetLastPrice() for test isolation

@drips-wave

drips-wave Bot commented Oct 20, 2026

Copy link
Copy Markdown

@actvabdulrahman Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this — the core idea is right and runDiscoveryCycle is a reasonable shape: diff the newly discovered target set against the watched one, count what was added and removed, and keep going on a per-pair error instead of aborting the whole sweep. The pools_added_total / pools_removed_total / pools_discovered_total split is the right instrumentation, and the discoveryCycle.test.ts cases cover the diff logic properly.

But it cannot merge as it stands, and the central reason is that the feature is never switched on.


1. Blocking: the rediscovery loop is dead code

startRediscovery is implemented on exactly one adapter (Aquarius), and nothing calls it:

$ git grep -n "startRediscovery" -- src/index.ts src/app.ts
(no matches)

So after this PR, src/index.ts starts the venue ingesters exactly as before, no rediscovery interval is ever created, runDiscoveryCycle never runs outside its own test, and the three new metrics stay at zero forever. Pool discovery is still not incremental — which is the whole of #220.

The interface declares it optional:

startRediscovery?(intervalMs: number, network?: NetworkName): Promise<void>

Optional plus unwired means nothing anywhere requires it to work. To close #220 the loop has to be started where the ingesters are started, for every adapter that implements it, with the interval coming from configuration (and documented in .env.example like PHOENIX_POOL_CACHE_TTL_MS is). If only Aquarius supports it for now, that is fine — but it must run for Aquarius.

2. Blocking: a second Phoenix adapter

src/ingest/venues/phoenix.ts is an add/add conflict: main already has a Phoenix adapter (merged from #460), so this adds a 153-line parallel implementation of the same venue. Your index.ts imports and re-exports yours — and then does not add it to the registry:

export const venueAdapters: VenueAdapter[] = [
  sdexAdapter,
  ammAdapter,
  soroswapAdapter,
  aquariusAdapter,
]   // phoenixAdapter imported, exported, never registered

So the duplicate adapter is also dead code. Please drop phoenix.ts and the index.ts import entirely and rebase on main; the Phoenix adapter there is already registered and already annotated for the generated README table.

3. Blocking: build artifacts and lockfile churn are committed

install_output.txt   Bin 0 -> 760 bytes
test_output.txt      Bin 0 -> 5498 bytes
package-lock.json    26 +++---

Please remove the two .txt captures — they are terminal output, they are committed as binary, and they will be stale the moment anyone else runs anything. And revert package-lock.json: this PR adds no dependency, so the only thing the lockfile change can do is conflict (it did) or quietly move a transitive version under an unrelated review.

4. (global as any).__rediscoveryTimers

const timer = setInterval(async () => { … }, intervalMs)
;(global as any).__rediscoveryTimers = (global as any).__rediscoveryTimers || []
;(global as any).__rediscoveryTimers.push(timer)

Three problems in four lines. Nothing ever reads this array, so no caller can stop a loop. It grows without bound — one entry per startRediscovery call, forever, each pinning a live interval. And global is shared across a test file's suites, so an interval started in one test keeps firing into the next, which is the kind of thing that shows up weeks later as a flaky unrelated test.

Return a stop function (or take the stopSignal that VenueAdapterLoopOptions already defines, which startVenueAdapterLoop uses for exactly this), and let the caller own the handle.

5. Test seams on the production interface

/** Get current watched set (for testing) */
getWatchedSet?(): Set<any>
/** Run a discovery cycle manually (for testing) */
runDiscoveryCycle?(network?: NetworkName): Promise<{ added: number; removed: number }>

A comment saying "for testing" on a public interface method is a sign the seam is in the wrong place. runDiscoveryCycle is already exported from runner.ts and your tests can call it directly with a fake discoverFn — which is what discoveryCycle.test.ts mostly does. Set<any> also gives up the type information the generic RediscoveryState<T> was carrying.

6. Unrelated changes

  • src/api/rest.ts adds l.venue to the /pools SELECT and src/api/schemas.ts adds a venue property. That is a change to a public response shape, it is nothing to do with #220, and it needs pool_snapshots.venue to exist plus its own openapi/client:gen regeneration. Please split it into its own PR — happy to review it there.
  • Trailing blank lines added to the end of rest.ts, schemas.ts and index.ts.

Where to pick it up

Rebase on main first — it has moved a long way since you cut this branch, and two things now interact with your change directly:

  • #471 landed a per-cycle deadline (src/ingest/cycleDeadline.ts). Adapter target discovery now receives an AbortSignal and is expected to check it at write-unit boundaries. A rediscovery loop should take the same signal rather than being a second unbounded loop beside the bounded one.
  • #475 made type and description required on VenueAdapter, so any adapter object needs both.

Keep runDiscoveryCycle and its tests — that part is good. Drop the duplicate adapter, the artifacts, the lockfile and the /pools change, replace the global timer array with a returned stop handle, and wire the loop where the ingesters start. That is a much smaller PR than this one and it would close the issue.

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.

Pool discovery is not incremental, so new pools appear only on restart

2 participants