refactor(router): cache precomputed sqrtPriceLimit values in init#1265
refactor(router): cache precomputed sqrtPriceLimit values in init#1265aronpark1007 wants to merge 2 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 43 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis change optimizes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
contract/r/gnoswap/router/v1/swap_inner.gno (1)
17-17: Import is necessary but creates inconsistency — useplv1.FeeTier*constants ingetMinTick/getMaxTickto matchinit().Line 17: The
plv1import is already used ininit()at line 35 to iterate overplv1.FeeTier100/500/3000/10000, but the same fee values are hardcoded as literalcasearms ingetMinTick/getMaxTick(lines 193–200, 213–220). Replace the literal case values withplv1.FeeTier*constants to maintain consistency and reduce duplication:Example fix for getMinTick
func getMinTick(fee uint32) int32 { switch fee { case plv1.FeeTier100: return -887272 case plv1.FeeTier500: return -887270 case plv1.FeeTier3000: return -887220 case plv1.FeeTier10000: return -887200 default: panic(...) } }No import cycle detected (pool/v1 does not import router/v1).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8092e1a-a895-46f2-a7f7-46af11842008
📒 Files selected for processing (1)
contract/r/gnoswap/router/v1/swap_inner.gno
| var ( | ||
| sqrtPriceLimitForward = make(map[uint32]*u256.Uint) // zeroForOne=true: sqrtRatioAtTick(minTick+1) + 1 | ||
| sqrtPriceLimitBackward = make(map[uint32]*u256.Uint) // zeroForOne=false: sqrtRatioAtTick(maxTick-1) - 1 | ||
| ) | ||
|
|
||
| func init() { | ||
| for _, fee := range []uint32{plv1.FeeTier100, plv1.FeeTier500, plv1.FeeTier3000, plv1.FeeTier10000} { | ||
| // zeroForOne=true: price must stay above minimum | ||
| minTick := getMinTick(fee) + 1 | ||
| fwd := common.TickMathGetSqrtRatioAtTick(minTick) // returns a fresh allocation | ||
| if fwd.IsZero() { | ||
| fwd = u256.MustFromDecimal(MIN_SQRT_RATIO) | ||
| } | ||
| fwd.Add(fwd, one) // fwd += 1 in-place | ||
| sqrtPriceLimitForward[fee] = fwd | ||
|
|
||
| // zeroForOne=false: price must stay below maximum | ||
| maxTick := getMaxTick(fee) - 1 | ||
| bwd := common.TickMathGetSqrtRatioAtTick(maxTick) // returns a fresh allocation | ||
| if bwd.IsZero() { | ||
| bwd = u256.MustFromDecimal(MAX_SQRT_RATIO) | ||
| } | ||
| bwd.Sub(bwd, one) // bwd -= 1 in-place | ||
| sqrtPriceLimitBackward[fee] = bwd | ||
| } | ||
| } |
There was a problem hiding this comment.
Cached pointers are mutable and aliased to callers — invariant is comment-only.
sqrtPriceLimitForward[fee] / sqrtPriceLimitBackward[fee] hold *u256.Uint, and calculateSqrtPriceLimitForSwap (Lines 183/185) returns that same pointer directly to callers. Today the only downstream use is .ToString() (Lines 81, 119), so it works — but u256.Uint exposes in-place mutators (Add, Sub, Set, etc.) and the init itself relies on that (fwd.Add(fwd, one)). A future refactor that, say, tweaks the limit before passing it to pl.Swap would silently corrupt the global cache for every subsequent swap at that fee tier, permanently — no panic, just wrong prices.
The "read-only" invariant is enforced solely by a comment. Consider returning a defensive clone on lookup, or storing the decimal string (which is what pl.Swap/pl.DrySwap actually consume) to eliminate the aliasing footgun. Caching the string also sidesteps the repeated ToString() cost on the hot path.
♻️ Option: cache the string form (no aliasing, no repeated ToString)
var (
- sqrtPriceLimitForward = make(map[uint32]*u256.Uint) // zeroForOne=true: sqrtRatioAtTick(minTick+1) + 1
- sqrtPriceLimitBackward = make(map[uint32]*u256.Uint) // zeroForOne=false: sqrtRatioAtTick(maxTick-1) - 1
+ sqrtPriceLimitForwardStr = make(map[uint32]string) // zeroForOne=true
+ sqrtPriceLimitBackwardStr = make(map[uint32]string) // zeroForOne=false
)…and have calculateSqrtPriceLimitForSwap return the string (adjusting swapInner/swapDryInner to skip the redundant ToString()). Alternatively, keep the map but return cached.Clone() on each lookup.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
aronpark1007
force-pushed
the
refactor/cache-sqrt-price-limit-in-init
branch
from
0f130ac to
ac29aa0
Compare
|
|
@jinoosss |
1 participant
Description
Precomputes TickMathGetSqrtRatioAtTick results for all fee tier and swap direction combinations in init() and caches them in a map, replacing per-swap tick math computation with a
simple map lookup.
Why This Change Is Needed
TickMathGetSqrtRatioAtTick involves heavy u256 bit arithmetic and was called on every swap to compute the sqrt price limit. Since the inputs are determined by a fixed set of 4 fee
tiers and 2 swap directions, all 8 combinations can be precomputed once at initialization. This was confirmed to be the largest gas reduction among the optimizations explored in
this series.
Test Plan
No new tests required. Existing tests cover all changes:
cache precomputed sqrtPriceLimit values in init
TestCalculateSqrtPriceLimitForSwap(swap_inner_test.gno) — validates basic behavior of calculateSqrtPriceLimitForSwapTestCalculateSqrtPriceLimitForSwap_ZeroForOne(swap_test.gno) — covers limit value accuracy per zeroForOne directionTestCalculateSqrtPriceLimitForSwap_AllFees(swap_test.gno) — validates all cached values across 4 fee tiers × 2 directions, covers unknown fee panic caseSummary by CodeRabbit