Repository navigation
#Closes 220 - #583
#Closes 220#583actvabdulrahman wants to merge 4 commits into
Conversation
|
@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! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
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 registeredSo 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.tsaddsl.venueto the/poolsSELECT andsrc/api/schemas.tsadds avenueproperty. That is a change to a public response shape, it is nothing to do with #220, and it needspool_snapshots.venueto exist plus its own openapi/client:genregeneration. Please split it into its own PR — happy to review it there.- Trailing blank lines added to the end of
rest.ts,schemas.tsandindex.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 anAbortSignaland 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
typeanddescriptionrequired onVenueAdapter, 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.
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
Checklist
I have read CONTRIBUTING.md
npx tsc --noEmitpassesnpm run buildpassesI 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_pointshistory is fully retainedpools_addedandpools_removedcounts exported as metrics per venue per cycleRe-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.tsreference structure including_resetLastPrice()for test isolation