johba/harb
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: Address review findings from PR #575

Browse source 
- Fix unsafe int32 intermediate cast: int56(int32(elapsed)) → int56(uint56(elapsed))
  to prevent TWAP tick sign inversion for intervals above int32 max (~68 years)
- Remove redundant lastRecenterTimestamp state variable; capture prevTimestamp
  from existing lastRecenterTime instead (saves ~20k gas per recenter)
- Use pool.increaseObservationCardinalityNext(ORACLE_CARDINALITY) in constructor
  instead of recomputing the pool address; extract magic 100 to named constant
- Add TWAPFallback(uint32 elapsed) event emitted when pool.observe() reverts
  so monitoring can distinguish degraded operation from normal bootstrap
- Remove conditional bypass paths in test_twapReflectsAveragePriceNotJustLastSwap;
  assert vwapAfter > 0 and vwapAfter > initialPriceX96 unconditionally

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
openhands 2026-03-11 10:02:47 +00:00
parent 83e43386bc
commit f72f99aefa
2 changed files with 30 additions and 50 deletions
Download patch file Download diff file Expand all files Collapse all files

24
onchain/src/LiquidityManager.sol
View file

@ -51,16 +51,19 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle {
/// @notice Last recenter tick — used to determine net trade direction between recenters /// @notice Last recenter tick — used to determine net trade direction between recenters
int24 public lastRecenterTick; int24 public lastRecenterTick;
/// @notice Last recenter timestamp — rate limits open (permissionless) recenters /// @notice Last recenter timestamp — rate limits open recenters and provides the previous
/// recenter time for TWAP interval calculations.
uint256 public lastRecenterTime; uint256 public lastRecenterTime;
/// @notice Minimum seconds between open recenters (when recenterAccess is unset) /// @notice Minimum seconds between open recenters (when recenterAccess is unset)
uint256 internal constant MIN_RECENTER_INTERVAL = 60; uint256 internal constant MIN_RECENTER_INTERVAL = 60;
/// @notice Target observation cardinality requested from the pool during construction
/// @notice Last recenter block timestamp — used to compute elapsed interval for pool TWAP oracle uint16 internal constant ORACLE_CARDINALITY = 100;
uint256 public lastRecenterTimestamp;
/// @notice Emitted on each successful recenter for monitoring and indexing /// @notice Emitted on each successful recenter for monitoring and indexing
event Recentered(int24 indexed currentTick, bool indexed isUp); event Recentered(int24 indexed currentTick, bool indexed isUp);
/// @notice Emitted when pool.observe() falls back to anchor midpoint; non-zero elapsed
/// indicates degraded oracle operation rather than normal bootstrap.
event TWAPFallback(uint32 elapsed);
/// @notice Custom errors /// @notice Custom errors
error ZeroAddressInSetter(); error ZeroAddressInSetter();
@ -88,7 +91,7 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle {
optimizer = Optimizer(_optimizer); optimizer = Optimizer(_optimizer);
// Increase observation cardinality so pool.observe() has sufficient history // Increase observation cardinality so pool.observe() has sufficient history
// for TWAP calculations between recenters. // for TWAP calculations between recenters.
IUniswapV3Pool(PoolAddress.computeAddress(factory, poolKey)).increaseObservationCardinalityNext(100); pool.increaseObservationCardinalityNext(ORACLE_CARDINALITY);
} }
/// @notice Callback function for Uniswap V3 mint operations /// @notice Callback function for Uniswap V3 mint operations
@ -147,9 +150,8 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle {
require(block.timestamp >= lastRecenterTime + MIN_RECENTER_INTERVAL, "recenter cooldown"); require(block.timestamp >= lastRecenterTime + MIN_RECENTER_INTERVAL, "recenter cooldown");
require(_isPriceStable(currentTick), "price deviated from oracle"); require(_isPriceStable(currentTick), "price deviated from oracle");
} }
uint256 prevTimestamp = lastRecenterTimestamp; uint256 prevTimestamp = lastRecenterTime;
lastRecenterTime = block.timestamp; lastRecenterTime = block.timestamp;
lastRecenterTimestamp = block.timestamp;
// Check if price movement is sufficient for recentering // Check if price movement is sufficient for recentering
isUp = false; isUp = false;
@ -283,7 +285,6 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle {
/// @return priceX96 Price in Q96 format (price * 2^96) /// @return priceX96 Price in Q96 format (price * 2^96)
function _getTWAPOrFallback(uint256 prevTimestamp, int24 tickLower, int24 tickUpper) function _getTWAPOrFallback(uint256 prevTimestamp, int24 tickLower, int24 tickUpper)
internal internal
view
returns (uint256 priceX96) returns (uint256 priceX96)
{ {
// Only attempt TWAP when there is a measurable elapsed interval // Only attempt TWAP when there is a measurable elapsed interval
@ -293,10 +294,13 @@ contract LiquidityManager is ThreePositionStrategy, PriceOracle {
secondsAgos[0] = elapsed; secondsAgos[0] = elapsed;
secondsAgos[1] = 0; secondsAgos[1] = 0;
try pool.observe(secondsAgos) returns (int56[] memory tickCumulatives, uint160[] memory) { try pool.observe(secondsAgos) returns (int56[] memory tickCumulatives, uint160[] memory) {
int24 twapTick = int24((tickCumulatives[1] - tickCumulatives[0]) / int56(int32(elapsed))); int24 twapTick = int24((tickCumulatives[1] - tickCumulatives[0]) / int56(uint56(elapsed)));
return _priceAtTick(token0isWeth ? -1 * twapTick : twapTick); return _priceAtTick(token0isWeth ? -1 * twapTick : twapTick);
} catch { } catch {
// Observation history not deep enough — fall through to anchor midpoint // pool.observe() failed — emit event so monitoring can distinguish
// degraded oracle operation from normal bootstrap (elapsed == 0).
emit TWAPFallback(elapsed);
// Fall through to anchor midpoint
} }
} }
// Fallback: anchor midpoint (original single-snapshot behaviour) // Fallback: anchor midpoint (original single-snapshot behaviour)

56
onchain/test/VWAPFloorProtection.t.sol
View file

@ -222,7 +222,7 @@ contract VWAPFloorProtectionTest is UniSwapHelper {
* not merely the last-swap anchor midpoint. * not merely the last-swap anchor midpoint.
* *
* Sequence: * Sequence:
* 1. First recenter → positions set, no fees (lastRecenterTimestamp = t0). * 1. First recenter → positions set, no fees (lastRecenterTime = t0).
* 2. Warp 100 s → buy KRK: price moves UP, observation written at t0+100. * 2. Warp 100 s → buy KRK: price moves UP, observation written at t0+100.
* 3. Warp 100 s → buy KRK again: price moves further UP, observation at t0+200. * 3. Warp 100 s → buy KRK again: price moves further UP, observation at t0+200.
* 4. Warp 100 s → bootstrap recenter (cumulativeVolume==0 → always records). * 4. Warp 100 s → bootstrap recenter (cumulativeVolume==0 → always records).
@ -240,8 +240,9 @@ contract VWAPFloorProtectionTest is UniSwapHelper {
// we track elapsed time with a local variable. // we track elapsed time with a local variable.
// Step 1: initial recenter — places positions at the pool's current price. // Step 1: initial recenter — places positions at the pool's current price.
// No fees yet; lastRecenterTimestamp is set to block.timestamp. // No fees yet; lastRecenterTime is set to block.timestamp.
(, int24 initialTick,,,,,) = pool.slot0(); (, int24 initialTick,,,,,) = pool.slot0();
assertFalse(token0isWeth, "test assumes token0isWeth=false");
vm.prank(RECENTER_CALLER); vm.prank(RECENTER_CALLER);
lm.recenter(); lm.recenter();
@ -261,10 +262,7 @@ contract VWAPFloorProtectionTest is UniSwapHelper {
// Capture the current (elevated) tick after two rounds of buying. // Capture the current (elevated) tick after two rounds of buying.
(, int24 elevatedTick,,,,,) = pool.slot0(); (, int24 elevatedTick,,,,,) = pool.slot0();
// The price must have risen — sanity check for !token0isWeth ordering.
// For !token0isWeth: buying KRK increases the tick (KRK price in WETH rises). // For !token0isWeth: buying KRK increases the tick (KRK price in WETH rises).
assertFalse(token0isWeth, "test assumes token0isWeth=false");
assertGt(elevatedTick, initialTick, "price must have risen after buys"); assertGt(elevatedTick, initialTick, "price must have risen after buys");
// Step 4: advance 100 s then do the bootstrap recenter. // Step 4: advance 100 s then do the bootstrap recenter.
@ -272,44 +270,22 @@ contract VWAPFloorProtectionTest is UniSwapHelper {
// elapsed = 300 s → pool.observe([300, 0]) → TWAP tick ≈ avg of three 100-s periods. // elapsed = 300 s → pool.observe([300, 0]) → TWAP tick ≈ avg of three 100-s periods.
t += 100; t += 100;
vm.warp(t); // t = 301 vm.warp(t); // t = 301
uint256 vwapBefore = lm.getVWAP();
vm.prank(RECENTER_CALLER); vm.prank(RECENTER_CALLER);
try lm.recenter() { lm.recenter();
uint256 vwapAfter = lm.getVWAP();
// If fees were collected, VWAP was updated. // TWAP over the 300-s window reflects higher prices than the initial anchor tick.
if (vwapAfter > 0 && vwapAfter != vwapBefore) { // TWAP tick ≈ (initialTick·100 + midTick·100 + elevatedTick·100) / 300 > initialTick.
// TWAP over the 300-s window reflects higher prices than the initial anchor tick. // Correspondingly, priceX96(TWAP) > priceX96(initialTick).
// The initial anchor was placed at `initialTick` (before any buys). //
// TWAP tick ≈ (initialTick·100 + midTick·100 + elevatedTick·100) / 300 > initialTick. // Compute a reference: the price at the initial anchor tick.
// Correspondingly, priceX96(TWAP) > priceX96(initialTick). // For !token0isWeth, _priceAtTick uses the tick directly (no negation).
// // We approximate it via TickMath: sqrtRatio² >> 96.
// Compute a reference: the price at the initial anchor tick. uint256 vwapAfter = lm.getVWAP();
// For !token0isWeth, _priceAtTick uses the tick directly (no negation). assertGt(vwapAfter, 0, "VWAP must be bootstrapped after fees from two large buys");
// We approximate it via TickMath: sqrtRatio² >> 96.
uint160 sqrtAtInitial = uint160(uint256(TickMath.getSqrtRatioAtTick(initialTick)));
uint256 initialPriceX96 = uint256(sqrtAtInitial) * uint256(sqrtAtInitial) >> 96;
assertGt( uint160 sqrtAtInitial = uint160(uint256(TickMath.getSqrtRatioAtTick(initialTick)));
vwapAfter, uint256 initialPriceX96 = uint256(sqrtAtInitial) * uint256(sqrtAtInitial) >> 96;
initialPriceX96, assertGt(vwapAfter, initialPriceX96, "TWAP VWAP must exceed initial-anchor-midpoint price");
"TWAP VWAP must exceed initial-anchor-midpoint price"
);
} else if (lm.cumulativeVolume() == 0) {
// No ETH fees collected: ethFee == 0 so _recordVolumeAndPrice was skipped.
// This can happen when feeDestination receives all fees before recording.
// Accept the result as long as VWAP is still 0 (nothing recorded yet).
assertEq(vwapAfter, 0, "VWAP still zero when no ETH fees collected");
}
} catch (bytes memory reason) {
// Only "amplitude not reached" is an acceptable failure — it means the second
// recenter couldn't detect sufficient price movement relative to the first one.
assertEq(
keccak256(reason),
keccak256(abi.encodeWithSignature("Error(string)", "amplitude not reached.")),
"unexpected revert in bootstrap recenter"
);
}
} }
// ========================================================================= // =========================================================================