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 all 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
15 changes: 6 additions & 9 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,14 +169,14 @@ 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)
swapStartHook(cross, pool.PoolPath(), blockTimestamp)
}
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

}

Expand Down Expand Up @@ -204,7 +204,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 +223,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 +318,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 +729,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
6 changes: 3 additions & 3 deletions tests/integration/testdata/deploy.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,9 @@ gnokey maketx addpkg -pkgdir ../../../examples/gno.land/r/gnoswap/gov/staker/v1
# stdout 'STORAGE DELTA: (2|3)[0-9]{5} bytes'
# stdout 'STORAGE FEE: (2|3)[0-9]{7}ugnot'

### deploy gov/governance (expected gas fee: 70918ugnot, gas wanted: 70918000)
gnokey maketx addpkg -pkgdir ../../../examples/gno.land/r/gnoswap/gov/governance/v1 -pkgpath gno.land/r/gnoswap/gov/governance/v1 -gas-fee 106377ugnot -gas-wanted 106377000 -broadcast -chainid=tendermint_test test1
# stdout 'GAS USED: 74[0-9]{6}'
### deploy gov/governance (expected gas fee: 110000ugnot, gas wanted: 110000000)
gnokey maketx addpkg -pkgdir ../../../examples/gno.land/r/gnoswap/gov/governance/v1 -pkgpath gno.land/r/gnoswap/gov/governance/v1 -gas-fee 165000ugnot -gas-wanted 165000000 -broadcast -chainid=tendermint_test test1
# stdout 'GAS USED: 1[0-1][0-9]{7}'
# stdout 'STORAGE DELTA: (2|3)[0-9]{5} bytes'
# stdout 'STORAGE FEE: (2|3)[0-9]{7}ugnot'

Expand Down
Loading