add support for Algebra Integral v1.2/v1.2.1#43
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughRenames the Algebra V2 base to AlgebraIntegralV1BaseProvider, adds AlgebraIntegralV1_2BaseProvider with ABI-driven event parsing and post-log reserve/tick refetch logic, migrates multiple liquidity providers to the new bases, and threads tick/activeTick through pool models and constructors. Changes
Sequence Diagram(s)sequenceDiagram
participant Logs as Logs (incoming)
participant Provider as AlgebraIntegralV1_2BaseProvider
participant RPC as RPC / Multicall
participant Pools as Pool Store
rect rgba(200,200,255,0.5)
Logs->>Provider: deliver logs (Swap/Mint/Burn/Flash/...)
end
rect rgba(200,255,200,0.5)
Provider->>Provider: parse logs -> identify affected pools & plugin-fee-updated pools
Provider->>RPC: batch fetch reserves + ticks for affected pools
RPC-->>Provider: return reserves + ticks
Provider->>Pools: update pool objects (reserves, ticks, liquidity, sqrtPriceX96, activeTick)
Provider->>Provider: enqueue/process pending tick updates as needed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/sushi/src/router/liquidity-providers/UniswapV3Base.ts (1)
383-394: 🧹 Nitpick | 🔵 TrivialConsider passing
pool.tickto theUniV3Poolconstructor for consistency with the Rain path.The Rain
UniswapV3Base.ts(line 231–232) passesundefined, pool.tickas the last two args toUniV3Pool, but this non-Rain call site omits them. Sincetickis optional on the constructor, this compiles fine andthis.tickwill beundefinedon the resulting pool instance.If no downstream code in the non-Rain path reads
UniV3Pool.tick, this is acceptable. Otherwise, consider passing it:♻️ Optional: pass tick for consistency
const v3Pool = new UniV3Pool( pool.address, pool.token0 as RToken, pool.token1 as RToken, pool.fee / 1_000_000, balance0, balance1, pool.activeTick, liquidity, pool.sqrtPriceX96, poolTicks, + undefined, + pool.tick, )packages/sushi/src/router/rain/UniswapV3Base.ts (1)
633-658:⚠️ Potential issue | 🟡 Minor
pool.tickis not updated in the Swap event handler, causing staleness.When a Swap event is processed,
pool.activeTickis recalculated from the event'stick(line 645–646), butpool.tick(the raw tick) is never updated. After a swap,pool.tickretains its initial value fromfetchPoolData.Since
getCurrentPoolList(line 232) passespool.tickto theUniV3Poolconstructor, subsequent pool list constructions will use a stale raw tick. Whilethis.tickisn't used inUniV3Pool's swap calculations (onlynearestTickandactiveTickmatter), it's a data consistency gap.🔧 Suggested fix
if (tick !== undefined) { + pool.tick = tick pool.activeTick = Math.floor(tick / pool.tickSpacing) * pool.tickSpacing
🤖 Fix all issues with AI agents
In `@packages/sushi/src/tines/RPool.ts`:
- Around line 55-56: The parameter types for tick and activeTick are
inconsistent: change the signature that declares activeTick?: number | undefined
to use the same optional shorthand activeTick?: number for consistency; locate
the function/constructor in RPool (symbol names around tick and activeTick) and
replace the verbose union type with the concise optional number type.
- Around line 43-44: The RPool base type defines tick?: number | undefined and
activeTick?: number | undefined which are specific to concentrated-liquidity
pools; move these properties out of RPool into the concentrated pool subtype
(e.g., the Concentrated or ConcentratedRPool class/interface) so only
concentrated pools carry them, or if you intentionally want them on every RPool
add a clarifying comment explaining that decision; also simplify the types by
removing the redundant "| undefined" since the optional marker already implies
undefined (change definitions like tick?: number).
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sushi/src/router/rain/RainDataFetcher.ts (1)
344-358:⚠️ Potential issue | 🟡 MinorIntermediate slice overlap fix is correct, but same-block re-call now throws instead of returning
false, and a one-block processing lag is introduced.The
+1n/-1npairing correctly converts the exclusive upper bound to an inclusive RPC argument. For intermediate slices this fixes a pre-existing overlap bug: before, consecutive slices covered[S, S+5n]and[S+5n, S+10n](6 blocks each, overlapping at the boundary); now they cover[S, S+4n]and[S+5n, S+9n](5 non-overlapping blocks each) ✓.However, storing
untilBlock + 1ninblockNumberSlicespropagates forward: when all slice promises settle, the result-processing loop at line 379 reassignsuntilBlock = blockNumberSlices[last] = original_untilBlock + 1n, and line 418 then setspool.blockNumber = original_untilBlock + 1n.This creates two issues:
Throw regression — After a successful
updatePools(N), a subsequent call with the same argument hasfromBlock = N + 1n > N = untilBlock, which falls through to the throw guard at line 327. PreviouslyfromBlock === N === untilBlockwas caught by the graceful early return at line 326. While this may not occur in typical sequential usage (where blocks always increment), it's a semantic regression for edge cases.One-block processing lag — Calling
updatePools(N + 1n)immediately after results infromBlock = N + 1n = untilBlock, hitting the early-return on line 326 (and while conditionN+1n < N+1nis also false). Block N+1's events are not processed untilupdatePools(N + 2n)is called. If this lag is intentional per the "fix reserve sync" commit message, it should be documented; otherwise, the boundary logic needs adjustment.For issue 1, the equality check should widen to handle re-processed case:
- if (fromBlock === untilBlock) return false + if (fromBlock >= untilBlock) return falseFor issue 2, clarify whether the one-block lag is intentional given the commit message context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sushi/src/router/rain/RainDataFetcher.ts` around lines 344 - 358, blockNumberSlices is storing the exclusive upper bound (toBlock = untilBlock + 1n) which later causes pool.blockNumber to be bumped by +1 and triggers the "fromBlock > untilBlock" throw in updatePools; fix this by pushing the inclusive upper bound into blockNumberSlices (push toBlock - 1n instead of toBlock) so later result-processing and pool.blockNumber use the original untilBlock, and also relax the early-return/throw logic in updatePools (the guard around fromBlock/ untilBlock) to treat fromBlock === untilBlock as a no-op/non-throw case (i.e. return false or handle gracefully) to avoid the regression when the same block is re-requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts`:
- Around line 189-191: The loop variable in the for...of over newTicks shadows
the outer variable tick (from event.args); rename the inner loop variable (e.g.,
newTick or candidateTick) in the block that pushes into queue[1] to avoid
shadowing and update any references inside that loop (see the for (const tick of
newTicks) block in AlgebraIntegralV1_2Base.ts).
- Around line 179-180: The code compares overrideFee and pluginFee to 0n without
undefined guards, which can throw if either is undefined; update the event
handler that destructures event.args so you first check overrideFee !==
undefined and pluginFee !== undefined (or coerce undefined to 0n) before doing
any BigInt comparison, and only call this.onSwapPluginFeeUpdatePools.push(pool)
when the guarded comparison confirms a value > 0n; reference the overrideFee and
pluginFee variables and the this.onSwapPluginFeeUpdatePools.push(pool) call when
making the change.
- Around line 160-198: The Swap branch currently skips updating pool.activeTick
when a plugin fee is present; always update pool.activeTick whenever tick !==
undefined regardless of overrideFee/pluginFee, then keep the onPoolTickChange()
call and newTicksQueue logic gated only to the no-plugin-fee path (so the lazy
tick loading remains unchanged); modify the Swap case in AlgebraIntegralV1_2Base
(referencing pool.activeTick, onPoolTickChange, onSwapPluginFeeUpdatePools, and
newTicksQueue) to set pool.activeTick = Math.floor(tick / pool.tickSpacing) *
pool.tickSpacing whenever tick is defined, and only push to
onSwapPluginFeeUpdatePools or run onPoolTickChange/newTicksQueue per the
existing fee check.
- Around line 5-29: The override for the eventsAbi property in
AlgebraIntegralV1_2BaseProvider should match the parent class' typing
pattern—change the assignment to use the AlgebraIntegralV1_2EventsAbi value cast
to any (i.e., set eventsAbi = AlgebraIntegralV1_2EventsAbi as any) so it aligns
with AlgebraIntegralV1BaseProvider and other providers; update the eventsAbi
override in the AlgebraIntegralV1_2BaseProvider class to use the as any cast
referencing AlgebraIntegralV1_2EventsAbi.
---
Outside diff comments:
In `@packages/sushi/src/router/rain/RainDataFetcher.ts`:
- Around line 344-358: blockNumberSlices is storing the exclusive upper bound
(toBlock = untilBlock + 1n) which later causes pool.blockNumber to be bumped by
+1 and triggers the "fromBlock > untilBlock" throw in updatePools; fix this by
pushing the inclusive upper bound into blockNumberSlices (push toBlock - 1n
instead of toBlock) so later result-processing and pool.blockNumber use the
original untilBlock, and also relax the early-return/throw logic in updatePools
(the guard around fromBlock/ untilBlock) to treat fromBlock === untilBlock as a
no-op/non-throw case (i.e. return false or handle gracefully) to avoid the
regression when the same block is re-requested.
packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts
Show resolved
Hide resolved
packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts
Show resolved
Hide resolved
packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts
Outdated
Show resolved
Hide resolved
packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts`:
- Around line 34-71: The bug is that this.onSwapPluginFeeUpdatePools is not
snapshotted before awaiting async fetches, so pools pushed during the awaits are
lost when the array is later cleared; fix by creating a local snapshot (e.g.,
const poolsSnapshot = [...this.onSwapPluginFeeUpdatePools.splice(0)] or
slice(0)) at the top of the flow and use poolsSnapshot when calling
getReserves/getTicks and iterating/assigning results (replace references to
this.onSwapPluginFeeUpdatePools in the getReserves/getTicks calls and the result
loop with poolsSnapshot), then clear only any remaining items from the original
this.onSwapPluginFeeUpdatePools or push back items that failed to fetch,
mirroring the newTicksQueue pattern used with getTicksInner.
In `@packages/sushi/src/router/rain/UniswapV3Base.ts`:
- Around line 231-232: The call passing a positional undefined into the
UniV3Pool constructor (the argument immediately before pool.tick) is unclear;
update the call site to clarify that this is the optional nearestTick parameter
— e.g., replace the bare undefined with an inline comment "undefined, //
nearestTick" or refactor the UniV3Pool constructor to accept an options object
and pass { nearestTick: undefined } (or the actual value) so the intent is
explicit; locate the invocation where pool.tick is passed and adjust the
argument there or change the UniV3Pool signature and all call sites accordingly.
---
Duplicate comments:
In `@packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts`:
- Line 179: The Swap handler currently compares overrideFee and pluginFee
directly to 0n which can throw if they are undefined; before any mutations to
pool (reserve0/reserve1/sqrtPriceX96/liquidity/activeTick) add undefined guards
for overrideFee and pluginFee (e.g., check !== undefined like the other
destructured event.args fields) and only perform the BigInt comparisons or treat
undefined as 0n after validation, ensuring you validate both values first and
bail or skip fee logic so the pool state is not mutated when these args are
missing.
- Around line 202-204: The loop in which "for (const tick of newTicks)" reuses
the name tick already destructured from event.args, causing variable shadowing;
rename the loop variable (e.g., newTick or pushedTick) in the block that pushes
into queue[1] and update any references inside that loop accordingly so it no
longer shadows the outer tick variable used elsewhere in the function (refer to
newTicks, queue, and the earlier event.args destructured tick).
- Line 29: The override of eventsAbi in AlgebraIntegralV1_2Base is too strictly
typed, causing parseEventLogs to infer event.args fields (e.g., overrideFee,
pluginFee) as bigint | undefined; fix it by casting the override to any like
other providers — change the declaration that sets AlgebraIntegralV1_2EventsAbi
in AlgebraIntegralV1_2Base (the eventsAbi override) to use an explicit `as any`
cast so parseEventLogs and usages of event.args (overrideFee, pluginFee) get the
expected bigint types.
packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts`:
- Around line 79-80: Remove the redundant non-null assertion on the result of
this.pools.get by changing the declaration of pool in AlgebraIntegralV1_2Base
(currently: const pool = this.pools.get(logAddress)!) to not use '!' so the
subsequent if (pool) guard is meaningful; in short, replace the asserted lookup
with a nullable one (const pool = this.pools.get(logAddress)) and keep the
existing if (pool) branch unchanged.
---
Duplicate comments:
In `@packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts`:
- Around line 28-29: The eventsAbi override in AlgebraIntegralV1_2BaseProvider
lacks the same type-cast used elsewhere causing TypeScript errors; update the
override to cast AlgebraIntegralV1_2EventsAbi with "as any" (matching the
pattern used in AlgebraIntegralV1BaseProvider and PancakeSwapV3Provider) so the
property signature aligns with consumers of this.eventsAbi; locate the override
on the AlgebraIntegralV1_2BaseProvider class and change the assignment of
eventsAbi to use the as any cast for AlgebraIntegralV1_2EventsAbi.
- Around line 33-71: The code is dropping pools added to
onSwapPluginFeeUpdatePools during awaiting of getReserves/getTicks; mirror the
newTicksQueue pattern by taking an atomic snapshot at the start (e.g. const
poolsSnapshot = [...this.onSwapPluginFeeUpdatePools.splice(0)]) and use
poolsSnapshot for the multicall inputs, for awaiting results, and for the
subsequent loop (replace references to this.onSwapPluginFeeUpdatePools with
poolsSnapshot) so pools pushed by handlePoolEvents during awaits are not lost;
finally, do not unconditionally clear this.onSwapPluginFeeUpdatePools (leave it
intact for new entries) — only clear or repush items from the snapshot as
needed.
- Line 179: Guard against undefined for event.args.pluginFee and
event.args.overrideFee before performing BigInt comparisons to avoid partial
pool updates; e.g., read const pluginFeeRaw = event.args.pluginFee and const
overrideFeeRaw = event.args.overrideFee, coerce with nullish fallback (pluginFee
= pluginFeeRaw ?? 0n, overrideFee = overrideFeeRaw ?? 0n) or explicitly check
(pluginFeeRaw !== undefined && pluginFeeRaw > 0n) before mutating the pool
state, then use those safe values in the existing pool-state mutation block and
subsequent logic so no mutations occur based on undefined values.
In `@packages/sushi/src/router/rain/UniswapV3Base.ts`:
- Around line 231-232: The positional bare undefined used for the nearestTick
argument when calling the constructor (the parameter before pool.tick) is
unclear and fragile; replace that bare undefined with an explicit placeholder
accompanied by an inline comment clarifying it is the nearestTick argument (or,
alternatively, pass a properly named variable/constant like nearestTick) so
future readers and refactors understand the intent in the call site in
UniswapV3Base (the parameter immediately preceding pool.tick).
packages/sushi/src/router/liquidity-providers/AlgebraIntegralV1_2Base.ts
Outdated
Show resolved
Hide resolved
2 participants
Motivation
This PR adds support for
Algebra Integral v1.2 and v1.2.1which are the same asAlgebra Integral v1 and v1.1but differ in 2 of its events with 1 item.Hydrex is v1.2/v1.2.1 which is fixed in the PR.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Improvements
Tests