Skip to content

refactor(pool): optimize swap hook checks and cache block timestamp #1267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aronpark1007 wants to merge 5 commits into
base: main
Choose a base branch
from
Open

refactor(pool): optimize swap hook checks and cache block timestamp #1267

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
41328cf
refactor(pool): remove redundant Has check before Get for swap hooks
aronpark1007 Apr 21, 2026
3b7ba04
perf(pool): cache block timestamp during swap
notJoon Apr 21, 2026
bfeac62
test(pool): fix false positive in swap hook uninitialized tests
aronpark1007 Apr 23, 2026
14d95ad
refactor(pool): use Has guard before direct Get call for swap hooks
aronpark1007 Apr 24, 2026
294d158
revert(pool): restore nil guard for swap hooks and deploy.txtar
aronpark1007 Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions contract/r/gnoswap/pool/store.gno
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,11 @@ func (s *poolStore) HasSwapStartHook() bool {
}

// GetSwapStartHook retrieves the swap start hook function.
// Returns nil if no hook is set.
func (s *poolStore) GetSwapStartHook() func(cur realm, poolPath string, timestamp int64) {
result, err := s.kvStore.Get(StoreKeySwapStartHook.String())
if err != nil {
panic(err)
return nil
}

swapStartHook, ok := result.(func(cur realm, poolPath string, timestamp int64))
Expand All @@ -200,10 +201,11 @@ func (s *poolStore) HasSwapEndHook() bool {
}

// 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
}
Comment on lines 179 to 207
Copy link
Copy Markdown
Member

@jinoosss jinoosss Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors occurring in the store are ignored.
This differs from how other query functions in the store are handled.

Copy link
Copy Markdown
Author

@aronpark1007 aronpark1007 Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

@aronpark1007 aronpark1007 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

14d95ad


swapEndHook, ok := result.(func(cur realm, poolPath string) error)
Expand Down
14 changes: 6 additions & 8 deletions contract/r/gnoswap/pool/store_test.gno
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,12 +462,11 @@ func TestStoreSetAndGetSwapStartHook(t *testing.T) {
},
},
{
name: "panic when getting uninitialized swap start hook",
name: "return nil when getting uninitialized swap start hook",
testFn: func(t *testing.T, ps IPoolStore) {
ps.GetSwapStartHook()
retrieved := ps.GetSwapStartHook()
uassert.Equal(t, nil, retrieved)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
},
shouldPanic: true,
panicMessage: "should panic when getting uninitialized swap start hook",
},
}

Expand Down Expand Up @@ -519,12 +518,11 @@ func TestStoreSetAndGetSwapEndHook(t *testing.T) {
},
},
{
name: "panic when getting uninitialized swap end hook",
name: "return nil when getting uninitialized swap end hook",
testFn: func(t *testing.T, ps IPoolStore) {
ps.GetSwapEndHook()
retrieved := ps.GetSwapEndHook()
uassert.Equal(t, nil, retrieved)
},
shouldPanic: true,
panicMessage: "should panic when getting uninitialized swap end hook",
},
}

Expand Down
33 changes: 11 additions & 22 deletions contract/r/gnoswap/pool/v1/swap.gno
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,15 +169,11 @@ func (i *poolV1) Swap(
slot0Start.SetUnlocked(false)
pool.SetSlot0(slot0Start)
startTick := pool.Slot0Tick()
blockTimestamp := time.Now().Unix()

// Call swap start hook if set
if i.store.HasSwapStartHook() {
swapStartHook := i.store.GetSwapStartHook()

if swapStartHook != nil {
currentTime := time.Now().Unix()
swapStartHook(cross, pool.PoolPath(), currentTime)
}
Comment on lines 175 to -180
Copy link
Copy Markdown
Member

@jinoosss jinoosss Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@aronpark1007 aronpark1007 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, rolled back all store-related changes, keeping only the block timestamp caching.
See revert commit: 294d158

if swapStartHook := i.store.GetSwapStartHook(); swapStartHook != nil {
swapStartHook(cross, pool.PoolPath(), blockTimestamp)
}

defer func() {
Expand All @@ -186,14 +182,10 @@ func (i *poolV1) Swap(
slot0End.SetUnlocked(true)
pool.SetSlot0(slot0End)

if i.store.HasSwapEndHook() {
swapEndHook := i.store.GetSwapEndHook()

if swapEndHook != nil {
err := swapEndHook(cross, pool.PoolPath())
if err != nil {
panic(err)
}
if swapEndHook := i.store.GetSwapEndHook(); swapEndHook != nil {
err := swapEndHook(cross, pool.PoolPath())
if err != nil {
panic(err)
}
}
}()
Expand All @@ -204,7 +196,7 @@ func (i *poolV1) Swap(
amounts := i256.MustFromDecimal(amountSpecified)
feeGrowthGlobalX128 := getFeeGrowthGlobal(pool, zeroForOne)
feeProtocol := getFeeProtocol(slot0Start, zeroForOne)
cache := newSwapCache(feeProtocol, pool.Liquidity().Clone())
cache := newSwapCache(feeProtocol, pool.Liquidity().Clone(), blockTimestamp)
state := newSwapState(amounts, feeGrowthGlobalX128, cache.liquidityStart.Clone(), slot0Start)

comp := SwapComputation{
Expand All @@ -223,9 +215,7 @@ func (i *poolV1) Swap(

// Update oracle BEFORE applying swap result (using pre-swap state)
if result.NewTick != pool.Slot0Tick() {
currentTime := time.Now().Unix()

err := writeObservationByPool(pool, currentTime, pool.Slot0Tick(), pool.Liquidity())
err := writeObservationByPool(pool, cache.blockTimestamp, pool.Slot0Tick(), pool.Liquidity())
if err != nil {
panic(err)
}
Expand Down Expand Up @@ -320,7 +310,7 @@ func (i *poolV1) DrySwap(
amounts := i256.MustFromDecimal(amountSpecified)
feeGrowthGlobalX128 := getFeeGrowthGlobal(poolSnapshot, zeroForOne)
feeProtocol := getFeeProtocol(slot0Start, zeroForOne)
cache := newSwapCache(feeProtocol, poolSnapshot.Liquidity().Clone())
cache := newSwapCache(feeProtocol, poolSnapshot.Liquidity().Clone(), time.Now().Unix())
state := newSwapState(amounts, feeGrowthGlobalX128, cache.liquidityStart, slot0Start)

comp := SwapComputation{
Expand Down Expand Up @@ -731,8 +721,7 @@ func (i *poolV1) tickTransition(step StepComputations, zeroForOne bool, state Sw
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)
}
Comment on lines 729 to 733
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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)
}
}

}

Expand Down
2 changes: 1 addition & 1 deletion contract/r/gnoswap/pool/v1/swap_test.gno
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1339,7 +1339,7 @@ func TestSwap_PriceLimitEdgeCase_ZeroAmount(t *testing.T) {
amounts := i256.MustFromDecimal(amountSpecified)
feeGrowthGlobalX128 := getFeeGrowthGlobal(pool, zeroForOne)
feeProtocol := getFeeProtocol(slot0Start, zeroForOne)
cache := newSwapCache(feeProtocol, pool.Liquidity().Clone())
cache := newSwapCache(feeProtocol, pool.Liquidity().Clone(), 0)
state := newSwapState(amounts, feeGrowthGlobalX128, cache.liquidityStart, slot0Start)

comp := SwapComputation{
Expand Down
5 changes: 2 additions & 3 deletions contract/r/gnoswap/pool/v1/type.gno
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package v1

import (
"time"

i256 "gno.land/p/gnoswap/int256"
u256 "gno.land/p/gnoswap/uint256"

Expand Down Expand Up @@ -72,11 +70,12 @@ type SwapCache struct {
func newSwapCache(
feeProtocol uint8,
liquidityStart *u256.Uint,
blockTimestamp int64,
) *SwapCache {
return &SwapCache{
feeProtocol: feeProtocol,
liquidityStart: liquidityStart,
blockTimestamp: time.Now().Unix(),
blockTimestamp: blockTimestamp,
tickCumulative: 0,
secondsPerLiquidityCumulativeX128: u256.Zero(),
computedLatestObservation: false,
Expand Down
Loading