From cf8e7ee6ee3ad1a2679e6ce58ca2e10a37f42658 Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 27 Feb 2026 12:02:29 +0000 Subject: [PATCH] fix: address review feedback on PositionTracker and StrategyExecutor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix fee attribution: distribute fees only to positions whose tick range contains the active tick at close time (in-range weight), not by raw liquidity. FLOOR is priced far below current tick and rarely earns fees; the old approach would over-credit it and corrupt capital-efficiency and net-P&L numbers. Fallback to raw-liquidity weighting with a WARN log when no position is in range. - Warn on first-close skip: when _closePosition finds no open record (first recenter, before any tracking), log [TRACKER][WARN] instead of silently returning so the gap is visible in reports. - Add tick range assertion: require() that the incoming close snapshot tick range matches the stored open record — a mismatch would mean IL is computed across different ranges (apples vs oranges). - Fix finalBlock accuracy: logSummary now calls tracker.logFinalSummary(tracker.lastNotifiedBlock()) instead of lastRecenterBlock, so the summary reflects the actual last replay block rather than potentially hundreds of blocks early. - Initialize lastRecenterBlock = block.number in StrategyExecutor constructor to defer the first recenter attempt by recenterInterval blocks and document the invariant. - Extract shared FormatLib: _str(uint256) and _istr(int256) were copy-pasted in both PositionTracker and StrategyExecutor. Extracted to FormatLib.sol internal library; both contracts now use `using FormatLib`. Co-Authored-By: Claude Sonnet 4.6 --- onchain/script/backtesting/FormatLib.sol | 33 ++++ .../script/backtesting/PositionTracker.sol | 167 +++++++++++------- .../script/backtesting/StrategyExecutor.sol | 65 +++---- 3 files changed, 159 insertions(+), 106 deletions(-) create mode 100644 onchain/script/backtesting/FormatLib.sol diff --git a/onchain/script/backtesting/FormatLib.sol b/onchain/script/backtesting/FormatLib.sol new file mode 100644 index 0000000..c3f593f --- /dev/null +++ b/onchain/script/backtesting/FormatLib.sol @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +pragma solidity ^0.8.19; + +/** + * @title FormatLib + * @notice Shared integer-to-string formatting helpers for backtesting log output. + * Extracted to avoid copy-pasting the same logic across PositionTracker + * and StrategyExecutor. + */ +library FormatLib { + /// @notice Format an unsigned integer as a decimal string. + function str(uint256 v) internal pure returns (string memory) { + if (v == 0) return "0"; + uint256 tmp = v; + uint256 len; + while (tmp != 0) { + len++; + tmp /= 10; + } + bytes memory buf = new bytes(len); + while (v != 0) { + buf[--len] = bytes1(uint8(48 + v % 10)); + v /= 10; + } + return string(buf); + } + + /// @notice Format a signed integer as a decimal string (prefixed with '-' if negative). + function istr(int256 v) internal pure returns (string memory) { + if (v >= 0) return str(uint256(v)); + return string.concat("-", str(uint256(-v))); + } +} diff --git a/onchain/script/backtesting/PositionTracker.sol b/onchain/script/backtesting/PositionTracker.sol index 266230e..149b672 100644 --- a/onchain/script/backtesting/PositionTracker.sol +++ b/onchain/script/backtesting/PositionTracker.sol @@ -1,9 +1,9 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity ^0.8.19; +import { FormatLib } from "./FormatLib.sol"; import { LiquidityAmounts } from "@aperture/uni-v3-lib/LiquidityAmounts.sol"; import { TickMath } from "@aperture/uni-v3-lib/TickMath.sol"; - import { Math } from "@openzeppelin/utils/math/Math.sol"; import { IUniswapV3Pool } from "@uniswap-v3-core/interfaces/IUniswapV3Pool.sol"; import { console2 } from "forge-std/console2.sol"; @@ -17,7 +17,10 @@ import { console2 } from "forge-std/console2.sol"; * - Tick range (tickLower, tickUpper) and liquidity * - Entry/exit block and timestamp * - Token amounts at entry vs exit (via Uniswap V3 LiquidityAmounts math) - * - Fees earned (proportional to each position's share of total liquidity) + * - Fees earned — attributed only to positions whose tick range contained the + * active tick at the time of close (the only positions that could have been + * accruing fees). Falls back to raw-liquidity weighting if no position is + * in range at close time (rare edge case). * - Impermanent loss vs holding the initial token amounts * - Net P&L = fees value + IL (IL is negative when LP underperforms HODL) * @@ -26,15 +29,20 @@ import { console2 } from "forge-std/console2.sol"; * - Cumulative IL and net P&L (token0 units) * - Rebalance count * - Time in range: % of notified blocks where Anchor tick range contains current tick - * - Capital efficiency numerator/denominator for offline calculation + * - Capital efficiency accumulators (feesToken0 / liqBlocks) for offline calculation * * All output lines carry a [TRACKER][TYPE] prefix for downstream parseability. * Cumulative P&L is logged every CUMULATIVE_LOG_INTERVAL blocks. * * Not a Script — no vm access. Uses console2 for output only. + * + * WARNING — backtesting use only: `recordRecenter` has no access control. Do not + * reuse this contract in production contexts where untrusted callers exist. */ contract PositionTracker { using Math for uint256; + using FormatLib for uint256; + using FormatLib for int256; // ------------------------------------------------------------------------- // Types @@ -167,6 +175,14 @@ contract PositionTracker { /** * @notice Record a successful recenter: close old positions, open new ones. + * + * Fee attribution: fees are distributed only to positions whose tick range + * contained the active tick at the moment of close — the only positions that + * could have been accruing fees in Uniswap V3. When no position is in range + * at close time (rare), attribution falls back to raw liquidity weighting and + * a warning is logged. + * + * @dev No access control — this contract is backtesting-only tooling. * @param oldPos Pre-recenter snapshot (positions being burned). * @param newPos Post-recenter snapshot (positions being minted). * @param feesWeth Total WETH fees collected across all positions this recenter. @@ -184,7 +200,7 @@ contract PositionTracker { ) external { - (uint160 sqrtPriceX96,,,,,,) = pool.slot0(); + (uint160 sqrtPriceX96, int24 currentTick,,,,,) = pool.slot0(); // Map WETH/KRK fees to pool token0/token1 based on pool ordering. uint256 fees0 = token0isWeth ? feesWeth : feesKrk; @@ -195,12 +211,26 @@ contract PositionTracker { totalFeesToken0 += _valueInToken0(fees0, fees1, sqrtPriceX96); rebalanceCount++; - uint256 totalOldLiq = uint256(oldPos.floorLiq) + uint256(oldPos.anchorLiq) + uint256(oldPos.discLiq); + // Uniswap V3 only accrues fees to a position when the active tick is + // inside its range. Weight fee attribution by in-range liquidity only. + uint256 fFloorWeight = _inRangeLiq(oldPos.floorLiq, oldPos.floorLo, oldPos.floorHi, currentTick); + uint256 fAnchorWeight = _inRangeLiq(oldPos.anchorLiq, oldPos.anchorLo, oldPos.anchorHi, currentTick); + uint256 fDiscWeight = _inRangeLiq(oldPos.discLiq, oldPos.discLo, oldPos.discHi, currentTick); + uint256 totalFeeWeight = fFloorWeight + fAnchorWeight + fDiscWeight; - // Close old positions (skip stages where old liquidity is zero or no open record exists). - _closePosition(0, oldPos.floorLiq, oldPos.floorLo, oldPos.floorHi, fees0, fees1, totalOldLiq, sqrtPriceX96, blockNum); - _closePosition(1, oldPos.anchorLiq, oldPos.anchorLo, oldPos.anchorHi, fees0, fees1, totalOldLiq, sqrtPriceX96, blockNum); - _closePosition(2, oldPos.discLiq, oldPos.discLo, oldPos.discHi, fees0, fees1, totalOldLiq, sqrtPriceX96, blockNum); + // Fallback to raw-liquidity weights when no position covers the current tick. + if (totalFeeWeight == 0) { + console2.log("[TRACKER][WARN] no position in range at close: fee attribution falling back to raw liquidity"); + fFloorWeight = oldPos.floorLiq; + fAnchorWeight = oldPos.anchorLiq; + fDiscWeight = oldPos.discLiq; + totalFeeWeight = uint256(oldPos.floorLiq) + uint256(oldPos.anchorLiq) + uint256(oldPos.discLiq); + } + + // Close old positions, passing per-position fee weights. + _closePosition(0, oldPos.floorLiq, oldPos.floorLo, oldPos.floorHi, fees0, fees1, fFloorWeight, totalFeeWeight, sqrtPriceX96, blockNum); + _closePosition(1, oldPos.anchorLiq, oldPos.anchorLo, oldPos.anchorHi, fees0, fees1, fAnchorWeight, totalFeeWeight, sqrtPriceX96, blockNum); + _closePosition(2, oldPos.discLiq, oldPos.discLo, oldPos.discHi, fees0, fees1, fDiscWeight, totalFeeWeight, sqrtPriceX96, blockNum); // Open new positions. _openPosition(0, newPos.floorLiq, newPos.floorLo, newPos.floorHi, sqrtPriceX96, blockNum, timestamp); @@ -229,18 +259,18 @@ contract PositionTracker { uint256 timeInRangeBps = blocksChecked > 0 ? (blocksAnchorInRange * 10_000) / blocksChecked : 0; console2.log("[TRACKER][SUMMARY] === Final P&L Summary ==="); - console2.log(string.concat("[TRACKER][SUMMARY] finalBlock=", _str(blockNum))); - console2.log(string.concat("[TRACKER][SUMMARY] rebalances=", _str(rebalanceCount))); + console2.log(string.concat("[TRACKER][SUMMARY] finalBlock=", blockNum.str())); + console2.log(string.concat("[TRACKER][SUMMARY] rebalances=", rebalanceCount.str())); console2.log( - string.concat("[TRACKER][SUMMARY] totalFees0=", _str(totalFees0), " totalFees1=", _str(totalFees1), " totalFeesToken0=", _str(totalFeesToken0)) + string.concat("[TRACKER][SUMMARY] totalFees0=", totalFees0.str(), " totalFees1=", totalFees1.str(), " totalFeesToken0=", totalFeesToken0.str()) ); - console2.log(string.concat("[TRACKER][SUMMARY] totalIL=", _istr(finalIL), " netPnL=", _istr(finalNetPnL), " (token0 units)")); + console2.log(string.concat("[TRACKER][SUMMARY] totalIL=", finalIL.istr(), " netPnL=", finalNetPnL.istr(), " (token0 units)")); console2.log( string.concat( - "[TRACKER][SUMMARY] timeInRange=", _str(timeInRangeBps), " bps blocksAnchorInRange=", _str(blocksAnchorInRange), "/", _str(blocksChecked) + "[TRACKER][SUMMARY] timeInRange=", timeInRangeBps.str(), " bps blocksAnchorInRange=", blocksAnchorInRange.str(), "/", blocksChecked.str() ) ); - console2.log(string.concat("[TRACKER][SUMMARY] capitalEff: feesToken0=", _str(totalFeesToken0), " liqBlocks=", _str(totalLiquidityBlocks))); + console2.log(string.concat("[TRACKER][SUMMARY] capitalEff: feesToken0=", totalFeesToken0.str(), " liqBlocks=", totalLiquidityBlocks.str())); } // ------------------------------------------------------------------------- @@ -275,19 +305,19 @@ contract PositionTracker { "[TRACKER][OPEN] stage=", _stageName(stageIdx), " block=", - _str(blockNum), + blockNum.str(), " ts=", - _str(timestamp), + timestamp.str(), " tick=[", - _istr(tickLower), + int256(tickLower).istr(), ",", - _istr(tickUpper), + int256(tickUpper).istr(), "] liq=", - _str(uint256(liquidity)), + uint256(liquidity).str(), " amt0=", - _str(amt0), + amt0.str(), " amt1=", - _str(amt1) + amt1.str() ) ); } @@ -299,22 +329,35 @@ contract PositionTracker { int24 tickUpper, uint256 fees0Total, uint256 fees1Total, - uint256 totalOldLiq, + uint256 posWeight, // fee weight for this position (0 = out-of-range) + uint256 totalWeight, // sum of fee weights across all positions uint160 sqrtPriceX96, uint256 blockNum ) internal { if (liquidity == 0) return; + OpenPosition storage pos = openPositions[stageIdx]; - if (!pos.active) return; // first ever close: no open record to match + if (!pos.active) { + // First recenter: no prior open record exists (LM deployed with positions + // already placed before tracking began). Log a warning so the gap is visible. + console2.log( + string.concat("[TRACKER][WARN] stage=", _stageName(stageIdx), " close skipped at block=", blockNum.str(), " (no open record: first recenter)") + ); + return; + } + + // Guard: the incoming snapshot must match the stored open record. + // A mismatch would mean IL is computed across mismatched tick ranges. + require(tickLower == pos.tickLower && tickUpper == pos.tickUpper, "PositionTracker: tick range mismatch"); (uint256 exitAmt0, uint256 exitAmt1) = _positionAmounts(liquidity, tickLower, tickUpper, sqrtPriceX96); - // Attribute fees proportional to this position's share of total liquidity. + // Attribute fees proportional to this position's in-range weight. uint256 posLiq = uint256(liquidity); - uint256 myFees0 = totalOldLiq > 0 ? fees0Total.mulDiv(posLiq, totalOldLiq) : 0; - uint256 myFees1 = totalOldLiq > 0 ? fees1Total.mulDiv(posLiq, totalOldLiq) : 0; + uint256 myFees0 = totalWeight > 0 ? fees0Total.mulDiv(posWeight, totalWeight) : 0; + uint256 myFees1 = totalWeight > 0 ? fees1Total.mulDiv(posWeight, totalWeight) : 0; // IL = LP exit value (ex-fees) − HODL value at exit price (both in token0 units). int256 il = _computeIL(pos.entryAmount0, pos.entryAmount1, exitAmt0, exitAmt1, sqrtPriceX96); @@ -332,34 +375,34 @@ contract PositionTracker { "[TRACKER][CLOSE] stage=", _stageName(stageIdx), " block=", - _str(blockNum), + blockNum.str(), " entryBlock=", - _str(pos.entryBlock), + pos.entryBlock.str(), " tick=[", - _istr(tickLower), + int256(tickLower).istr(), ",", - _istr(tickUpper), + int256(tickUpper).istr(), "] liq=", - _str(posLiq) + posLiq.str() ) ); console2.log( string.concat( "[TRACKER][CLOSE] entryAmt0=", - _str(pos.entryAmount0), + pos.entryAmount0.str(), " entryAmt1=", - _str(pos.entryAmount1), + pos.entryAmount1.str(), " exitAmt0=", - _str(exitAmt0), + exitAmt0.str(), " exitAmt1=", - _str(exitAmt1), + exitAmt1.str(), " fees0=", - _str(myFees0), + myFees0.str(), " fees1=", - _str(myFees1) + myFees1.str() ) ); - console2.log(string.concat("[TRACKER][CLOSE] IL=", _istr(il), " netPnL=", _istr(netPnL), " (token0 units)")); + console2.log(string.concat("[TRACKER][CLOSE] IL=", il.istr(), " netPnL=", netPnL.istr(), " (token0 units)")); delete openPositions[stageIdx]; } @@ -369,19 +412,19 @@ contract PositionTracker { console2.log( string.concat( "[TRACKER][CUMULATIVE] block=", - _str(blockNum), + blockNum.str(), " rebalances=", - _str(rebalanceCount), + rebalanceCount.str(), " totalFees0=", - _str(totalFees0), + totalFees0.str(), " totalFees1=", - _str(totalFees1), + totalFees1.str(), " IL=", - _istr(totalILToken0), + totalILToken0.istr(), " netPnL=", - _istr(totalNetPnLToken0), + totalNetPnLToken0.istr(), " timeInRange=", - _str(timeInRangeBps), + timeInRangeBps.str(), " bps" ) ); @@ -391,6 +434,15 @@ contract PositionTracker { // Internal: Uniswap V3 math // ------------------------------------------------------------------------- + /** + * @notice Return `liquidity` if currentTick is inside [tickLo, tickHi), else 0. + * Used to attribute fees only to positions that were potentially in range. + */ + function _inRangeLiq(uint128 liquidity, int24 tickLo, int24 tickHi, int24 currentTick) internal pure returns (uint256) { + if (liquidity == 0) return 0; + return (currentTick >= tickLo && currentTick < tickHi) ? uint256(liquidity) : 0; + } + /** * @notice Compute (amount0, amount1) for a liquidity position at the given sqrt price. */ @@ -433,7 +485,7 @@ contract PositionTracker { } // ------------------------------------------------------------------------- - // Formatting helpers (no vm dependency) + // Formatting helper // ------------------------------------------------------------------------- function _stageName(uint8 idx) internal pure returns (string memory) { @@ -441,25 +493,4 @@ contract PositionTracker { if (idx == 1) return "ANCHOR"; return "DISC"; } - - function _str(uint256 v) internal pure returns (string memory) { - if (v == 0) return "0"; - uint256 tmp = v; - uint256 len; - while (tmp != 0) { - len++; - tmp /= 10; - } - bytes memory buf = new bytes(len); - while (v != 0) { - buf[--len] = bytes1(uint8(48 + v % 10)); - v /= 10; - } - return string(buf); - } - - function _istr(int256 v) internal pure returns (string memory) { - if (v >= 0) return _str(uint256(v)); - return string.concat("-", _str(uint256(-v))); - } } diff --git a/onchain/script/backtesting/StrategyExecutor.sol b/onchain/script/backtesting/StrategyExecutor.sol index 584a3a6..ed78b1d 100644 --- a/onchain/script/backtesting/StrategyExecutor.sol +++ b/onchain/script/backtesting/StrategyExecutor.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.19; import { LiquidityManager } from "../../src/LiquidityManager.sol"; import { ThreePositionStrategy } from "../../src/abstracts/ThreePositionStrategy.sol"; +import { FormatLib } from "./FormatLib.sol"; import { PositionTracker } from "./PositionTracker.sol"; import { IERC20 } from "@openzeppelin/token/ERC20/IERC20.sol"; import { IUniswapV3Pool } from "@uniswap-v3-core/interfaces/IUniswapV3Pool.sol"; @@ -33,6 +34,9 @@ import { console2 } from "forge-std/console2.sol"; * amounts based on KrAIken's active tick ranges. */ contract StrategyExecutor { + using FormatLib for uint256; + using FormatLib for int256; + // ------------------------------------------------------------------------- // Configuration (immutable) // ------------------------------------------------------------------------- @@ -50,6 +54,10 @@ contract StrategyExecutor { // Runtime state // ------------------------------------------------------------------------- + /// @notice Block of the last recenter attempt (successful or not). + /// Initialised to block.number at construction so the first recenter + /// attempt is deferred by recenterInterval blocks rather than firing + /// immediately on the first observed historical block. uint256 public lastRecenterBlock; uint256 public totalRecenters; uint256 public failedRecenters; @@ -73,6 +81,9 @@ contract StrategyExecutor { feeDestination = _feeDestination; recenterInterval = _recenterInterval; tracker = new PositionTracker(_pool, _token0isWeth); + // Defer the first recenter attempt by recenterInterval blocks so we don't + // try to recenter before any meaningful price movement has occurred. + lastRecenterBlock = block.number; } // ------------------------------------------------------------------------- @@ -118,7 +129,7 @@ contract StrategyExecutor { } if (!success) { - console2.log(string.concat("[recenter SKIP @ block ", _str(blockNum), "] reason: ", failReason)); + console2.log(string.concat("[recenter SKIP @ block ", blockNum.str(), "] reason: ", failReason)); return; } @@ -201,11 +212,14 @@ contract StrategyExecutor { console2.log("Total recenters: ", totalRecenters); console2.log("Failed recenters: ", failedRecenters); console2.log("Recenter interval:", recenterInterval, "blocks"); - console2.log(string.concat("Final Floor: tick [", _istr(fLo), ", ", _istr(fHi), "] liq=", _str(uint256(fLiq)))); - console2.log(string.concat("Final Anchor: tick [", _istr(aLo), ", ", _istr(aHi), "] liq=", _str(uint256(aLiq)))); - console2.log(string.concat("Final Discovery: tick [", _istr(dLo), ", ", _istr(dHi), "] liq=", _str(uint256(dLiq)))); + console2.log(string.concat("Final Floor: tick [", int256(fLo).istr(), ", ", int256(fHi).istr(), "] liq=", uint256(fLiq).str())); + console2.log(string.concat("Final Anchor: tick [", int256(aLo).istr(), ", ", int256(aHi).istr(), "] liq=", uint256(aLiq).str())); + console2.log(string.concat("Final Discovery: tick [", int256(dLo).istr(), ", ", int256(dHi).istr(), "] liq=", uint256(dLiq).str())); - tracker.logFinalSummary(lastRecenterBlock); + // Use lastNotifiedBlock from the tracker as the authoritative final block — + // it reflects the last block actually processed by the replay, which may be + // up to recenterInterval blocks later than lastRecenterBlock. + tracker.logFinalSummary(tracker.lastNotifiedBlock()); } // ------------------------------------------------------------------------- @@ -238,38 +252,13 @@ contract StrategyExecutor { internal view { - console2.log(string.concat("=== Recenter #", _str(totalRecenters), " @ block ", _str(blockNum), " ===")); - console2.log(string.concat(" Floor pre: tick [", _istr(fLoPre), ", ", _istr(fHiPre), "] liq=", _str(uint256(fLiqPre)))); - console2.log(string.concat(" Anchor pre: tick [", _istr(aLoPre), ", ", _istr(aHiPre), "] liq=", _str(uint256(aLiqPre)))); - console2.log(string.concat(" Disc pre: tick [", _istr(dLoPre), ", ", _istr(dHiPre), "] liq=", _str(uint256(dLiqPre)))); - console2.log(string.concat(" Floor post: tick [", _istr(fLoPost), ", ", _istr(fHiPost), "] liq=", _str(uint256(fLiqPost)))); - console2.log(string.concat(" Anchor post: tick [", _istr(aLoPost), ", ", _istr(aHiPost), "] liq=", _str(uint256(aLiqPost)))); - console2.log(string.concat(" Disc post: tick [", _istr(dLoPost), ", ", _istr(dHiPost), "] liq=", _str(uint256(dLiqPost)))); - console2.log(string.concat(" Fees WETH: ", _str(feesWeth), " Fees KRK: ", _str(feesKrk))); - } - - // ------------------------------------------------------------------------- - // Formatting helpers (no vm dependency required) - // ------------------------------------------------------------------------- - - function _str(uint256 v) internal pure returns (string memory) { - if (v == 0) return "0"; - uint256 tmp = v; - uint256 len; - while (tmp != 0) { - len++; - tmp /= 10; - } - bytes memory buf = new bytes(len); - while (v != 0) { - buf[--len] = bytes1(uint8(48 + v % 10)); - v /= 10; - } - return string(buf); - } - - function _istr(int256 v) internal pure returns (string memory) { - if (v >= 0) return _str(uint256(v)); - return string.concat("-", _str(uint256(-v))); + console2.log(string.concat("=== Recenter #", totalRecenters.str(), " @ block ", blockNum.str(), " ===")); + console2.log(string.concat(" Floor pre: tick [", int256(fLoPre).istr(), ", ", int256(fHiPre).istr(), "] liq=", uint256(fLiqPre).str())); + console2.log(string.concat(" Anchor pre: tick [", int256(aLoPre).istr(), ", ", int256(aHiPre).istr(), "] liq=", uint256(aLiqPre).str())); + console2.log(string.concat(" Disc pre: tick [", int256(dLoPre).istr(), ", ", int256(dHiPre).istr(), "] liq=", uint256(dLiqPre).str())); + console2.log(string.concat(" Floor post: tick [", int256(fLoPost).istr(), ", ", int256(fHiPost).istr(), "] liq=", uint256(fLiqPost).str())); + console2.log(string.concat(" Anchor post: tick [", int256(aLoPost).istr(), ", ", int256(aHiPost).istr(), "] liq=", uint256(aLiqPost).str())); + console2.log(string.concat(" Disc post: tick [", int256(dLoPost).istr(), ", ", int256(dHiPost).istr(), "] liq=", uint256(dLiqPost).str())); + console2.log(string.concat(" Fees WETH: ", feesWeth.str(), " Fees KRK: ", feesKrk.str())); } }