refactor(pool): optimize swap hook checks and cache block timestamp#1267
refactor(pool): optimize swap hook checks and cache block timestamp#1267aronpark1007 wants to merge 5 commits intomainfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughCapture a single swap-start Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SwapEngine as Swap
participant Cache
participant Oracle
participant TickHook
Client->>Swap: initiate Swap request
Swap->>Swap: capture blockTimestamp
Swap->>Cache: newSwapCache(..., blockTimestamp)
Swap->>Oracle: write observation(using cache.blockTimestamp)
Swap->>TickHook: invoke tickCross(tick, cache.blockTimestamp)
TickHook-->>Swap: tick hook result
Oracle-->>Swap: confirmation
Swap-->>Client: swap result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/r/gnoswap/pool/store.gno (1)
177-217:⚠️ Potential issue | 🔴 CriticalUpdate test expectations in
pool/store_test.gno— GetSwapStartHook and GetSwapEndHook now return nil instead of panicking on uninitialized access.The test cases at lines 465-471 ("panic when getting uninitialized swap start hook") and lines 522-528 ("panic when getting uninitialized swap end hook") declare
shouldPanic: true, but the new implementation returnsnilon KV-retrieval errors rather than panicking. These tests will fail and must be rewritten to assertnilreturns instead.Note: The actual usage in
swap.gno(lines 175–176, 185–186) already guards calls withif swapStartHook != nilandif swapEndHook != nilchecks, so the nil-return behavior is compatible with the Swap flow.
🧹 Nitpick comments (2)
contract/r/gnoswap/pool/store.gno (1)
229-242: Inconsistency:GetTickCrossHookstill panics on KV error.This PR relaxes
GetSwapStartHook/GetSwapEndHookto returnnilon KV-retrieval errors, butGetTickCrossHookat lines 229-242 is left panicking. The only current caller incontract/r/gnoswap/pool/v1/swap.gno(lines 721-722) still guards withHasTickCrossHook()first, so it is safe today, but the asymmetry is easy to misuse if a future caller drops theHascheck here the same way swap-start/end just did. Consider either mirroring the nil-on-error behavior (and documenting it) or keeping all three symmetric on panic — whichever the PR intended.contract/r/gnoswap/pool/v1/swap.gno (1)
313-313: Minor:DrySwapstill callstime.Now().Unix()inline — consider for consistency.Not a defect (DrySwap is read-only and short-lived, and the cache's timestamp is unused for any observable DrySwap output in current tests), but if the intent of the PR is to standardize a single timestamp per swap invocation, you could hoist this to a local variable the same way
Swapdoes at line 172 for stylistic consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e20c315a-271d-467e-b0b5-de01df31bb65
📒 Files selected for processing (4)
contract/r/gnoswap/pool/store.gnocontract/r/gnoswap/pool/v1/swap.gnocontract/r/gnoswap/pool/v1/swap_test.gnocontract/r/gnoswap/pool/v1/type.gno
GetSwapStartHook / GetSwapEndHook now return nil instead of panicking when uninitialized. Previous shouldPanic tests were false positives due to uassert.NotEqual not handling nil function type comparison correctly.
jinoosss
left a comment
jinoosss
left a comment
There was a problem hiding this comment.
The changes to swapCache look fine.
However, the naming convention for functions that query hooks is changing, which may differ from the convention used in existing functions.
| result, err := s.kvStore.Get(StoreKeySwapStartHook.String()) | ||
| if err != nil { | ||
| panic(err) | ||
| return nil | ||
| } | ||
|
|
||
| swapStartHook, ok := result.(func(cur realm, poolPath string, timestamp int64)) | ||
| if !ok { | ||
| panic(ufmt.Sprintf("failed to cast result to func(poolPath string, timestamp int64): %T", result)) | ||
| } | ||
|
|
||
| return swapStartHook | ||
| } | ||
|
|
||
| // SetSwapStartHook stores the swap start hook function. | ||
| func (s *poolStore) SetSwapStartHook(swapStartHook func(cur realm, poolPath string, timestamp int64)) error { | ||
| return s.kvStore.Set(StoreKeySwapStartHook.String(), swapStartHook) | ||
| } | ||
|
|
||
| // HasSwapEndHook checks if the swap end hook is set. | ||
| func (s *poolStore) HasSwapEndHook() bool { | ||
| return s.kvStore.Has(StoreKeySwapEndHook.String()) | ||
| } | ||
|
|
||
| // GetSwapEndHook retrieves the swap end hook function. | ||
| // Returns nil if no hook is set. | ||
| func (s *poolStore) GetSwapEndHook() func(cur realm, poolPath string) error { | ||
| result, err := s.kvStore.Get(StoreKeySwapEndHook.String()) | ||
| if err != nil { | ||
| panic(err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Errors occurring in the store are ignored.
This differs from how other query functions in the store are handled.
There was a problem hiding this comment.
Instead of changing the store's error handling, we can keep GetSwapStartHook / GetSwapEndHook panicking on error and simplify the call site by trusting Has* as the guard:
if i.store.HasSwapStartHook() {
i.store.GetSwapStartHook()(cross, pool.PoolPath(), blockTimestamp)
}Since Has* already confirms the key exists, the nil check after Get* is unnecessary. This removes the redundant double-check without touching the store's error handling convention.
How about reverting the store.gno change and updating the call site this way instead?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contract/r/gnoswap/pool/v1/swap.gno (1)
312-312: Minor:DrySwapleft out of the caching pattern.
DrySwapstill callstime.Now().Unix()inline when constructing the cache whileSwapnow hoists it to a namedblockTimestamplocal. It is only used once here so there's no correctness/performance concern, but introducing the same local would keep the two call sites stylistically consistent and make future reuse (e.g., if DrySwap ever grows oracle/hook calls) trivial.♻️ Optional tiny consistency tweak
feeProtocol := getFeeProtocol(slot0Start, zeroForOne) - cache := newSwapCache(feeProtocol, poolSnapshot.Liquidity().Clone(), time.Now().Unix()) + blockTimestamp := time.Now().Unix() + cache := newSwapCache(feeProtocol, poolSnapshot.Liquidity().Clone(), blockTimestamp)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b74525a4-a194-4401-972c-a547f879ff53
📒 Files selected for processing (1)
contract/r/gnoswap/pool/v1/swap.gno
| if i.store.HasSwapStartHook() { | ||
| swapStartHook := i.store.GetSwapStartHook() | ||
|
|
||
| if swapStartHook != nil { | ||
| currentTime := time.Now().Unix() | ||
| swapStartHook(cross, pool.PoolPath(), currentTime) | ||
| } |
There was a problem hiding this comment.
The function type of hook can be nil.
When actually using the function, it would be best to add a nil check for defensive programming.
There was a problem hiding this comment.
As discussed, rolled back all store-related changes, keeping only the block timestamp caching.
See revert commit: 294d158
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contract/r/gnoswap/pool/v1/swap.gno (1)
321-321: Optional: mirror theblockTimestampcaching inDrySwapfor consistency.
Swapnow capturesblockTimestamp := time.Now().Unix()once and passes it tonewSwapCache, whileDrySwapstill inlinestime.Now().Unix()in the constructor call. Functionally equivalent in Gno (block time is constant within a transaction), but it makesDrySwapdiverge stylistically fromSwapand obscures the fact that both paths should observe the same moment in simulated execution. Consider hoisting the call for readability/symmetry.♻️ Proposed refactor
feeProtocol := getFeeProtocol(slot0Start, zeroForOne) - cache := newSwapCache(feeProtocol, poolSnapshot.Liquidity().Clone(), time.Now().Unix()) + blockTimestamp := time.Now().Unix() + cache := newSwapCache(feeProtocol, poolSnapshot.Liquidity().Clone(), blockTimestamp)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cfdfe2a-56e7-4bad-9229-063e74be814a
📒 Files selected for processing (2)
contract/r/gnoswap/pool/v1/swap.gnotests/integration/testdata/deploy.txtar
✅ Files skipped from review due to trivial changes (1)
- tests/integration/testdata/deploy.txtar
| if i.store.HasTickCrossHook() { | ||
| tickCrossHook := i.store.GetTickCrossHook() | ||
|
|
||
| currentTime := time.Now().Unix() | ||
| tickCrossHook(cross, pool.PoolPath(), step.tickNext, zeroForOne, currentTime) | ||
| tickCrossHook(cross, pool.PoolPath(), step.tickNext, zeroForOne, cache.blockTimestamp) | ||
| } |
There was a problem hiding this comment.
Add a nil guard on tickCrossHook for consistency with swap start/end hooks.
SwapStartHook (lines 178) and SwapEndHook (line 192) both defensively check != nil after Get...(), but tickCrossHook is invoked directly after GetTickCrossHook() without the same guard. Per the PR objectives ("A revert commit restores the nil guard for swap hooks"), this path looks like it was missed in the revert. If Has...() and Get...() ever go out of sync (e.g., hook was set to nil without clearing the "has" flag), this will panic mid-swap after state has already been partially mutated and a reentrancy lock acquired — hard to recover from.
🛡️ Proposed fix
if i.store.HasTickCrossHook() {
tickCrossHook := i.store.GetTickCrossHook()
-
- tickCrossHook(cross, pool.PoolPath(), step.tickNext, zeroForOne, cache.blockTimestamp)
+ if tickCrossHook != nil {
+ tickCrossHook(cross, pool.PoolPath(), step.tickNext, zeroForOne, cache.blockTimestamp)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if i.store.HasTickCrossHook() { | |
| tickCrossHook := i.store.GetTickCrossHook() | |
| currentTime := time.Now().Unix() | |
| tickCrossHook(cross, pool.PoolPath(), step.tickNext, zeroForOne, currentTime) | |
| tickCrossHook(cross, pool.PoolPath(), step.tickNext, zeroForOne, cache.blockTimestamp) | |
| } | |
| if i.store.HasTickCrossHook() { | |
| tickCrossHook := i.store.GetTickCrossHook() | |
| if tickCrossHook != nil { | |
| tickCrossHook(cross, pool.PoolPath(), step.tickNext, zeroForOne, cache.blockTimestamp) | |
| } | |
| } |
3 participants
Summary
Hascheck beforeGetfor swap hooks —Getalready handles the missing case by returning niltime.Now()on every tick cross during the swap loopTest Plan
No new tests required. Existing tests cover all changes:
remove redundant Has check before Get for swap hooks
TestSwap_ExecutionAndSecurity— covers Swap() execution with nil hook, naturally validates nil return behaviorcache block timestamp during swap
TestOracle_SwapScenarios— covers oracle write timestamp path inside swapTestOracle_TWAPConsistencyCases— covers TWAP timestamp consistency including tick crossingTestSwap_PriceLimitEdgeCase_ZeroAmount— reflects newSwapCache signature change (already updated to pass 0)Summary by CodeRabbit