Conversation
SushiSwap V3 pools emit the same CLMM shape Soroswap's already do —
topics [Symbol("swap")], signed amount0/amount1 — so the extractor is
made venue-neutral (TokenPair / PairPoolRegistry / PairSwapExtractor,
with the Soroswap* names kept as aliases) rather than copied. Only the
venue stamped on the TradeRow differs.
learn_factory gains a pool_created arm. It matches on shape like every
other arm, so the three earlier factory generations whose pools still
trade register through the same path as the live factory — no factory
address is hardcoded anywhere.
The two pair-backed venues keep separate registries, so a contract_id
can never resolve to the other venue's tokens.
unregistered_pool_venue must key on the DATA, not the topic: SushiSwap's
routers emit the identical [Symbol("swap")] topic and differ only in
carrying amount_in/amount_out against the pool's amount0/amount1.
Matching the topic alone would count every routed swap as a missing
pool — the double-counting shape 0285 flagged.
--discover-pools read only the Aquarius, Phoenix and Soroswap factory events, so it could never seed a SushiSwap pool. pool_created has a Symbol topic, so signature carries it; adding it to the filter is enough. With no emitter filter one read covers all four factory generations. Checked on production over ledgers 60M-65M: 133 pools announced, every contract on the two known pool wasms among them. Another protocol's token-less pool_created is read too and pinned as learning nothing.
A routed trade emits two swap events in one transaction, the pool's and the router's summary of the same trade. The test drives a real one (ledger 64,481,111) through process_soroban_event_rows and requires a single tick, no registered router, and no missing-pool count. A router wrongly registered as a pool still adds no tick. The seed runbook now lists pool_created and SushiSwap's earlier start.
karczuRF
left a comment
karczuRF
left a comment
There was a problem hiding this comment.
Code review — correctness pass over the SushiSwap V3 indexing diff
What was verified. cargo test --workspace --no-run builds clean; cargo test -p prices-ingest-core -p soroswap-extractor -p ledger-processor -p events-backfill --lib is green, including all six new SushiSwap tests; the CI clippy set (cargo clippy -p extractors-core -p phoenix-extractor -p soroswap-extractor -p aquarius-extractor -p ledger-processor -p prices-api -- -D warnings) passes.
The core decode holds up. The Uniswap-v3 sign convention (a0 >= 0 -> token0 in / |a1| out, else token1 in / |a0| out) is correct in both directions against the real production payloads. The router/pool discrimination on amount0 + amount1 is sound: all nine amount0-bearing samples in lore/4-notes/samples/soroban-events/swap.jsonl are the SushiSwap pool CCR2CH4G..., and no Aquarius or Soroswap event carries those keys, so the new match arm cannot misfire. The two pair registries really are disjoint, Venue is matched exhaustively at every site, and both pool_registry.venue and price_ohlcv_*.source are LowCardinality(String) rather than enums, so the new label inserts fine. The rollup MVs carry no source allowlist.
Four findings are left as inline comments. Two more are below, because they fall on lines this PR does not touch — GitHub cannot attach an inline comment to a line outside the diff — but they are direct consequences of adding the new source label, so they should not be dropped:
1. packages/prices-clickhouse/schema/preroll-amm-reprice.sql:229 (and ~24 further statements in the same file) — medium
Every preroll statement hardcodes:
WHERE t.source IN ('aquarius', 'phoenix', 'soroswap')This script is what re-rolls _15m / _1h / _4h / _1d / _1w / _1M from price_ohlcv_1m after an AMM history reprice. Once AC5's backfill writes source = 'sushiswap' into _1m and the operator runs this preroll, every sushiswap candle is silently omitted from all coarse tables. Since AMM history is read from _1d/_1h rather than _1m, the backfilled history would be invisible to consumers while _1m itself looks correct. 'sushiswap' needs adding to the filter before the history run.
2. packages/prices-clickhouse/schema/init.sql:608 — low
The pool_registry header comment still reads venue = 'soroswap' | 'phoenix' | 'aquarius' and describes token0/token1 as "the Soroswap pair tokens". This PR starts writing 'sushiswap' rows whose tokens come from pool_created, so the schema comment operators read is now stale.
Checked and cleared rather than flagged. The SushiSwap routers do land in out.unresolved — they emit a bare swap topic from an unregistered contract, and is_aquarius_router_swap does not exclude them. That is pre-existing and harmless here: the live processor discards out.unresolved entirely, and the events-backfill reprice reads only venue-known contract ids, so the routers never reach prices.unresolved_pools through either path.
| soroswap_registry: &SoroswapPoolRegistry, | ||
| sushiswap_registry: &SoroswapPoolRegistry, |
There was a problem hiding this comment.
low/medium — these two adjacent parameters have the identical type: SoroswapPoolRegistry is now just an alias of PairPoolRegistry, so soroswap_registry and sushiswap_registry are indistinguishable to the compiler.
Transposing them at a call site compiles silently and produces no runtime error: every Soroswap pool would resolve against SushiSwap's token table and vice versa, emitting candles for the wrong asset pair with no dispatch error and no unresolved record to notice it by.
The existing call site in classify_amm_groups is correct, but the separation the doc comment promises just above ("a contract_id can never resolve to the wrong venue's tokens") is enforced by argument order alone. A newtype wrapper per venue, or passing &Registries and letting dispatch pick the field, would make the mix-up unrepresentable.
| row.token1.clone(), | ||
| ); | ||
| } | ||
| Venue::Sushiswap => { |
There was a problem hiding this comment.
low — this arm registers row.token0 / row.token1 unconditionally, including when both are empty.
A pool_registry row with venue='sushiswap' and blank tokens therefore makes reg.sushiswap.contains() return true, which flips pair_unresolved to false in classify_amm_groups. The pool is then dispatched and priced against asset "" instead of being recorded in unresolved — the silent drop that guard exists to prevent.
This is exactly the hazard a_pool_created_without_a_token_pair_learns_nothing guards on the learn side; the load side has no equivalent check. Only reachable today via a hand-written or imported row (it mirrors the pre-existing Soroswap arm), hence low — but a if !row.token0.is_empty() && !row.token1.is_empty() guard here would close it for both venues.
| /// SushiSwap V3 — a Uniswap-v3-style concentrated-liquidity venue (task | ||
| /// 0290). Its pool `swap` carries signed `amount0`/`amount1`, the same | ||
| /// shape [`Venue::Soroswap`]'s CLMM pools use, so both decode through | ||
| /// [`TokenPair`]-backed extraction. |
There was a problem hiding this comment.
low — [TokenPair] cannot resolve from here. TokenPair lives in soroswap-extractor, which depends on extractors-core, not the reverse, so there is no path to it from this crate's docs.
Confirmed: cargo doc -p extractors-core --no-deps emits
warning: unresolved link to `TokenPair`
--> packages/extractors-core/src/lib.rs:11:11
= note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default
Not a CI failure (there is no cargo doc step), but it renders as a dead link. Plain backticks, or naming soroswap_extractor::TokenPair in prose, fixes it.
| catch-up only needs `63000000` to the tip. | ||
| catch-up only needs `63000000` to the tip. **Task 0290 is the exception:** | ||
| SushiSwap V3's pools go back to ledger 60,147,305, so its run starts at | ||
| `60000000` — the exact command is in the 0290 task file. |
There was a problem hiding this comment.
low — dead reference. lore/1-tasks/active/0290_FEATURE_index-the-uniswap-v3-style-clmm-venue.md contains no command; its only mention of the tool is the phrase "reuses [[0291]]'s --discover-pools shape".
So an operator following this runbook lands on nothing for the one venue the runbook singles out as the exception to the 63000000 start. Either inline the runnable command here (start 60000000, real values filled in, as the other runbook commands are) or add it to the task file before this merges.
Every statement in the AMM reprice pre-roll scoped itself to
`source IN ('aquarius', 'phoenix', 'soroswap')`, so once the backfill
writes sushiswap history into price_ohlcv_1m the re-roll would have
dropped every one of those rows from _15m/_1h/_4h/_1d/_1w/_1M. AMM
history is read from _1d/_1h rather than _1m, so the loss would have
been invisible on the table that looked correct.
Pins the filter against the canonical Venue list: adding a variant now
breaks a test instead of a backfill.
`soroswap_registry` and `sushiswap_registry` were adjacent parameters of the same type, so transposing them compiled silently and priced every Soroswap pool against SushiSwap's token table and vice versa — no error, just candles for the wrong asset pair. They now travel as one PairRegistries with named fields, built only by Registries::pair_registries.
A pool_registry row naming a pair-backed venue with empty token0/token1 was registered anyway, so contains() answered true, classify_amm_groups cleared pair_unresolved, and the pool was priced against asset "" instead of being recorded in unresolved_pools. It now loads its venue only — the same outcome the learn side already gives a pool_created with no pair.
- seed-pool-registry: the SushiSwap run pointed at a command the 0290 task file does not carry; the command is now inline, with its range and why one read covers every factory generation - init.sql: pool_registry's header listed three venues and called token0/token1 the Soroswap pair; both are now accurate - extractors-core: the Sushiswap doc linked TokenPair, which lives in a downstream crate and could not resolve
1 participant
Summary
Venue::Sushiswap(source = "sushiswap") — the venue behind factoryCD3KRKGD…GLYF, identified on-chain from its deployer'sSushiSwap V3 Positions NFT-V1contracts. 88,689 swaps across 99 pools since 2026-01 currently reach no candle.amount0/amount1under a bareswaptopic), sosoroswap-extractorbecomes venue-neutral (TokenPair/PairPoolRegistry/PairSwapExtractor) with theSoroswap*names kept as aliases. No call site changed; only the venue stamped on theTradeRowdiffers.learn_factorygains onepool_createdarm. It matches on shape like every other arm, so all four deployed factory generations register through it — 46 of the 99 traded pools come from the three earlier ones, and 3 of those still trade.contract_idcan never resolve to the other venue's tokens.unregistered_pool_venuekeys on the data (amount0+amount1), not the topic: the two routers emit the identical[Symbol("swap")]topic and differ only in carryingamount_in/amount_out. Matching the topic alone would count every routed swap as a missing pool.Draft — still open: extending
--discover-poolsto thepool_createdshape, and the history backfill. Nothing here can deploy until 0286 phase 1 lands (see task 0291).Tests: 1,118 passing (+3). Negative controls: removing the factory arm fails the factory test; keying the matcher on the topic alone fails both router tests.