From 54e9db883e64390c5c89bedf4e3dbbedb161ef2d Mon Sep 17 00:00:00 2001 From: Sara Reynolds Date: Wed, 26 Jun 2024 18:49:35 -0400 Subject: [PATCH 1/5] update decrease --- .../autocompound_exactUnclaimedFees.snap | 2 +- ...exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .../autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc20.snap | 2 +- .../decreaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .../increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- contracts/base/BaseLiquidityManagement.sol | 90 ++++++------------- contracts/libraries/CurrencySenderLibrary.sol | 30 ------- .../libraries/LiquidityDeltaAccounting.sol | 28 ++++++ contracts/libraries/Position.sol | 9 ++ 12 files changed, 72 insertions(+), 101 deletions(-) delete mode 100644 contracts/libraries/CurrencySenderLibrary.sol create mode 100644 contracts/libraries/LiquidityDeltaAccounting.sol diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 40ad7ac8..51e79ca7 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -258477 \ No newline at end of file +258556 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index e2e7eb05..62694eee 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -190850 \ No newline at end of file +190929 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index bcf9757d..45e19b9a 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -279016 \ No newline at end of file +279095 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index ae013013..de734ea6 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1 +1 @@ -190026 \ No newline at end of file +133896 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index 4d5e683a..d5b067f6 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1 +1 @@ -168894 \ No newline at end of file +121839 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index 4ea517e8..0b214a9f 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -171241 \ No newline at end of file +171342 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index c2e421fa..d714a47f 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -146823 \ No newline at end of file +146924 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index d2591995..4f29dd0f 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -466530 \ No newline at end of file +466631 \ No newline at end of file diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index bc9ab1da..61dbba13 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -16,7 +16,6 @@ import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol"; import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; import {CurrencySettleTake} from "../libraries/CurrencySettleTake.sol"; -import {CurrencySenderLibrary} from "../libraries/CurrencySenderLibrary.sol"; import {CurrencyDeltas} from "../libraries/CurrencyDeltas.sol"; import {FeeMath} from "../libraries/FeeMath.sol"; @@ -24,6 +23,7 @@ import {LiquiditySaltLibrary} from "../libraries/LiquiditySaltLibrary.sol"; import {IBaseLiquidityManagement} from "../interfaces/IBaseLiquidityManagement.sol"; import {PositionLibrary} from "../libraries/Position.sol"; import {BalanceDeltaExtensionLibrary} from "../libraries/BalanceDeltaExtensionLibrary.sol"; +import {LiquidityDeltaAccounting} from "../libraries/LiquidityDeltaAccounting.sol"; import "forge-std/console2.sol"; @@ -31,7 +31,6 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { using LiquidityRangeIdLibrary for LiquidityRange; using CurrencyLibrary for Currency; using CurrencySettleTake for Currency; - using CurrencySenderLibrary for Currency; using CurrencyDeltas for IPoolManager; using PoolIdLibrary for PoolKey; using StateLibrary for IPoolManager; @@ -40,6 +39,7 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { using LiquiditySaltLibrary for IHooks; using PositionLibrary for IBaseLiquidityManagement.Position; using BalanceDeltaExtensionLibrary for BalanceDelta; + using LiquidityDeltaAccounting for BalanceDelta; mapping(address owner => mapping(LiquidityRangeId rangeId => Position)) public positions; @@ -58,10 +58,10 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { // We always 0 out a caller positive delta because it is instead accounted for in position.tokensOwed. if (callerDelta0 < 0) currency0.settle(manager, owner, uint256(int256(-callerDelta0)), claims); - else if (callerDelta0 > 0) currency0.send(manager, owner, uint128(callerDelta0), claims); + else if (callerDelta0 > 0) currency0.take(manager, owner, uint128(callerDelta0), claims); if (callerDelta1 < 0) currency1.settle(manager, owner, uint256(int256(-callerDelta1)), claims); - else if (callerDelta1 > 0) currency1.send(manager, owner, uint128(callerDelta1), claims); + else if (callerDelta1 > 0) currency1.take(manager, owner, uint128(callerDelta1), claims); } function _unlockCallback(bytes calldata data) internal override returns (bytes memory) { @@ -115,33 +115,13 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { Position storage position = positions[owner][range.toId()]; + // Calculates the fee growth since the last time the positions feeGrowthInside was updated. + // Also updates the feeGrowthInsideLast variables in storage. + (BalanceDelta callerFeesAccrued) = _updateFeeGrowth(range, position); + // Calculate the portion of the liquidityDelta that is attributable to the caller. // We must account for fees that might be owed to other users on the same range. - (uint256 feeGrowthInside0X128, uint256 feeGrowthInside1X128) = - manager.getFeeGrowthInside(range.poolKey.toId(), range.tickLower, range.tickUpper); - - BalanceDelta callerFeesAccrued = FeeMath.getFeesOwed( - feeGrowthInside0X128, - feeGrowthInside1X128, - position.feeGrowthInside0LastX128, - position.feeGrowthInside1LastX128, - position.liquidity - ); - - if (totalFeesAccrued == callerFeesAccrued) { - // when totalFeesAccrued == callerFeesAccrued, the caller is not sharing the range - // therefore, the caller is responsible for the entire liquidityDelta - callerDelta = liquidityDelta; - } else { - // the delta for increasing liquidity assuming that totalFeesAccrued was not applied - BalanceDelta principalDelta = liquidityDelta - totalFeesAccrued; - - // outstanding deltas the caller is responsible for, after their fees are credited to the principal delta - callerDelta = principalDelta + callerFeesAccrued; - - // outstanding deltas this contract is responsible for, intuitively the contract is responsible for taking fees external to the caller's accrued fees - thisDelta = totalFeesAccrued - callerFeesAccrued; - } + (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); // Update position storage, flushing the callerDelta value to tokensOwed first if necessary. // If callerDelta > 0, then even after investing callerFeesAccrued, the caller still has some amount to collect that were not added into the position so they are accounted to tokensOwed and removed from the final callerDelta returned. @@ -158,7 +138,6 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { position.addTokensOwed(tokensOwed); position.addLiquidity(liquidityToAdd); - position.updateFeeGrowthInside(feeGrowthInside0X128, feeGrowthInside1X128); } function _increaseLiquidityAndZeroOut( @@ -236,41 +215,27 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { ); } + /// @dev Note that we do not update tokensOwed since all fees collected from this modify call are automatically sent to the caller. + /// Any outstanding amounts owed to the caller from tokensOwed0/tokensOwed1 must be collected explicitly. function _decreaseLiquidity( address owner, LiquidityRange memory range, uint256 liquidityToRemove, bytes memory hookData - ) internal returns (BalanceDelta delta) { + ) internal returns (BalanceDelta callerDelta, BalanceDelta thisDelta) { (BalanceDelta liquidityDelta, BalanceDelta totalFeesAccrued) = _modifyLiquidity(owner, range, -(liquidityToRemove.toInt256()), hookData); - // take all tokens first - // do NOT take tokens directly to the owner because this contract might be holding fees - // that need to be paid out (position.tokensOwed) - if (liquidityDelta.amount0() > 0) { - range.poolKey.currency0.take(manager, address(this), uint128(liquidityDelta.amount0()), true); - } - if (liquidityDelta.amount1() > 0) { - range.poolKey.currency1.take(manager, address(this), uint128(liquidityDelta.amount1()), true); - } - - // when decreasing liquidity, the user collects: 1) principal liquidity, 2) new fees, 3) old fees (position.tokensOwed) - Position storage position = positions[owner][range.toId()]; - BalanceDelta callerFeesAccrued = _updateFeeGrowth(range, position); - BalanceDelta principalDelta = liquidityDelta - totalFeesAccrued; - // new fees = new fees + old fees + principal liquidity - callerFeesAccrued = callerFeesAccrued - + toBalanceDelta(uint256(position.tokensOwed0).toInt128(), uint256(position.tokensOwed1).toInt128()) - + principalDelta; + // Calculates the fee growth since the last time the positions feeGrowthInside was updated + // Also updates the position's the feeGrowthInsideLast variables in storage. + (BalanceDelta callerFeesAccrued) = _updateFeeGrowth(range, position); - position.tokensOwed0 = 0; - position.tokensOwed1 = 0; - position.liquidity -= liquidityToRemove; + // Account for fees accrued to other users on the same range. + (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); - return callerFeesAccrued; + position.subtractLiquidity(liquidityToRemove); } function _decreaseLiquidityAndZeroOut( @@ -279,10 +244,12 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { uint256 liquidityToRemove, bytes memory hookData, bool claims - ) internal returns (BalanceDelta delta) { - delta = _decreaseLiquidity(owner, range, liquidityToRemove, hookData); - _closeCallerDeltas(delta, range.poolKey.currency0, range.poolKey.currency1, owner, claims); - _closeAllDeltas(range.poolKey.currency0, range.poolKey.currency1); + ) internal returns (BalanceDelta callerDelta) { + // Todo move to transient storage, and potentially bubble up both deltas + BalanceDelta thisDelta; + (callerDelta, thisDelta) = _decreaseLiquidity(owner, range, liquidityToRemove, hookData); + _closeCallerDeltas(callerDelta, range.poolKey.currency0, range.poolKey.currency1, owner, claims); + _closeThisDeltas(thisDelta, range.poolKey.currency0, range.poolKey.currency1); } function _lockAndDecreaseLiquidity( @@ -344,16 +311,14 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { ); } - // TODO: I deprecated this bc I liked to see the accounting in line in the top level function... and I like to do all the position updates at once. - // can keep but should at at least use the position library in here. function _updateFeeGrowth(LiquidityRange memory range, Position storage position) internal - returns (BalanceDelta _feesOwed) + returns (BalanceDelta callerFeesAccrued) { (uint256 feeGrowthInside0X128, uint256 feeGrowthInside1X128) = manager.getFeeGrowthInside(range.poolKey.toId(), range.tickLower, range.tickUpper); - _feesOwed = FeeMath.getFeesOwed( + callerFeesAccrued = FeeMath.getFeesOwed( feeGrowthInside0X128, feeGrowthInside1X128, position.feeGrowthInside0LastX128, @@ -361,8 +326,7 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { position.liquidity ); - position.feeGrowthInside0LastX128 = feeGrowthInside0X128; - position.feeGrowthInside1LastX128 = feeGrowthInside1X128; + position.updateFeeGrowthInside(feeGrowthInside0X128, feeGrowthInside1X128); } // --- View Functions --- // diff --git a/contracts/libraries/CurrencySenderLibrary.sol b/contracts/libraries/CurrencySenderLibrary.sol deleted file mode 100644 index 656a9439..00000000 --- a/contracts/libraries/CurrencySenderLibrary.sol +++ /dev/null @@ -1,30 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-or-later -pragma solidity ^0.8.24; - -import {Currency, CurrencyLibrary} from "v4-core/types/Currency.sol"; -import {CurrencySettleTake} from "./CurrencySettleTake.sol"; -import {IPoolManager} from "v4-core/interfaces/IPoolManager.sol"; - -/// @notice Library used to send Currencies from address to address -library CurrencySenderLibrary { - using CurrencyLibrary for Currency; - using CurrencySettleTake for Currency; - - /// @notice Send a custodied Currency to a recipient - /// @dev If sending ERC20 or native, the PoolManager must be unlocked - /// @param currency The Currency to send - /// @param manager The PoolManager - /// @param recipient The recipient address - /// @param amount The amount to send - /// @param useClaims If true, transfer ERC-6909 tokens - function send(Currency currency, IPoolManager manager, address recipient, uint256 amount, bool useClaims) - internal - { - if (useClaims) { - manager.transfer(recipient, currency.toId(), amount); - } else { - // currency.settle(manager, address(this), amount, true); // sends in tokens into PM from this address - currency.take(manager, recipient, amount, false); // takes out tokens from PM to recipient - } - } -} diff --git a/contracts/libraries/LiquidityDeltaAccounting.sol b/contracts/libraries/LiquidityDeltaAccounting.sol new file mode 100644 index 00000000..b6c99b10 --- /dev/null +++ b/contracts/libraries/LiquidityDeltaAccounting.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.20; + +import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; + +import "forge-std/console2.sol"; + +library LiquidityDeltaAccounting { + function split(BalanceDelta liquidityDelta, BalanceDelta callerFeesAccrued, BalanceDelta totalFeesAccrued) + internal + returns (BalanceDelta callerDelta, BalanceDelta thisDelta) + { + if (totalFeesAccrued == callerFeesAccrued) { + // when totalFeesAccrued == callerFeesAccrued, the caller is not sharing the range + // therefore, the caller is responsible for the entire liquidityDelta + callerDelta = liquidityDelta; + } else { + // the delta for increasing liquidity assuming that totalFeesAccrued was not applied + BalanceDelta principalDelta = liquidityDelta - totalFeesAccrued; + + // outstanding deltas the caller is responsible for, after their fees are credited to the principal delta + callerDelta = principalDelta + callerFeesAccrued; + + // outstanding deltas this contract is responsible for, intuitively the contract is responsible for taking fees external to the caller's accrued fees + thisDelta = totalFeesAccrued - callerFeesAccrued; + } + } +} diff --git a/contracts/libraries/Position.sol b/contracts/libraries/Position.sol index 79cd02c0..a3a099ba 100644 --- a/contracts/libraries/Position.sol +++ b/contracts/libraries/Position.sol @@ -6,6 +6,8 @@ import {BalanceDelta} from "v4-core/types/BalanceDelta.sol"; // Updates Position storage library PositionLibrary { + error InsufficientLiquidity(); + // TODO ensure this is one sstore. function addTokensOwed(IBaseLiquidityManagement.Position storage position, BalanceDelta tokensOwed) internal { position.tokensOwed0 += uint128(tokensOwed.amount0()); @@ -18,6 +20,13 @@ library PositionLibrary { } } + function subtractLiquidity(IBaseLiquidityManagement.Position storage position, uint256 liquidity) internal { + if (position.liquidity < liquidity) revert InsufficientLiquidity(); + unchecked { + position.liquidity -= liquidity; + } + } + // TODO ensure this is one sstore. function updateFeeGrowthInside( IBaseLiquidityManagement.Position storage position, From 932ee94a177baa9d538a0166a5c9d99caae747ec Mon Sep 17 00:00:00 2001 From: Sara Reynolds Date: Thu, 27 Jun 2024 17:08:47 -0400 Subject: [PATCH 2/5] update collect --- .../autocompound_exactUnclaimedFees.snap | 2 +- ...exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .../autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc20.snap | 2 +- .../decreaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .../increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- contracts/base/BaseLiquidityManagement.sol | 43 ++++++++++--------- contracts/libraries/Position.sol | 5 +++ 10 files changed, 35 insertions(+), 29 deletions(-) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 51e79ca7..20308207 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -258556 \ No newline at end of file +258574 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index 62694eee..be27e821 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -190929 \ No newline at end of file +190947 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index 45e19b9a..3632f8d2 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -279095 \ No newline at end of file +279113 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index de734ea6..95f17313 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1 +1 @@ -133896 \ No newline at end of file +133893 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index d5b067f6..91878896 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1 +1 @@ -121839 \ No newline at end of file +121836 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index 0b214a9f..cee1a41b 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -171342 \ No newline at end of file +171338 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index d714a47f..677231e7 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -146924 \ No newline at end of file +146920 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index 4f29dd0f..65812e8b 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -466631 \ No newline at end of file +466627 \ No newline at end of file diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index 61dbba13..d07eb1cd 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -267,39 +267,40 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { function _collect(address owner, LiquidityRange memory range, bytes memory hookData) internal - returns (BalanceDelta) + returns (BalanceDelta callerDelta, BalanceDelta thisDelta) { - (, BalanceDelta totalFeesAccrued) = _modifyLiquidity(owner, range, 0, hookData); + // Do not add or decrease liquidity, just trigger fee updates. + (BalanceDelta liquidityDelta, BalanceDelta totalFeesAccrued) = _modifyLiquidity(owner, range, 0, hookData); - PoolKey memory key = range.poolKey; Position storage position = positions[owner][range.toId()]; - // take all fees first then distribute - if (totalFeesAccrued.amount0() > 0) { - key.currency0.take(manager, address(this), uint128(totalFeesAccrued.amount0()), true); - } - if (totalFeesAccrued.amount1() > 0) { - key.currency1.take(manager, address(this), uint128(totalFeesAccrued.amount1()), true); - } + // Also updates the position's the feeGrowthInsideLast variables in storage. + (BalanceDelta callerFeesAccrued) = _updateFeeGrowth(range, position); - // collecting fees: new fees and old fees - BalanceDelta callerFeesAccrued = _updateFeeGrowth(range, position); - callerFeesAccrued = callerFeesAccrued - + toBalanceDelta(uint256(position.tokensOwed0).toInt128(), uint256(position.tokensOwed1).toInt128()); + // Account for fees accrued to other users on the same range. + // TODO: Opt when liquidityDelta == 0 + (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); - position.tokensOwed0 = 0; - position.tokensOwed1 = 0; + // Allow the caller to collect the tokens owed. + // Tokens owed that the caller collects is paid for by this contract. + // ie. Transfer the tokensOwed amounts to the caller from the position manager through the pool manager. + // TODO case where this contract does not have enough credits to pay the caller? + BalanceDelta tokensOwed = + toBalanceDelta(uint256(position.tokensOwed0).toInt128(), uint256(position.tokensOwed1).toInt128()); + callerDelta = callerDelta + tokensOwed; + thisDelta = thisDelta - tokensOwed; - return callerFeesAccrued; + position.clearTokensOwed(); } function _collectAndZeroOut(address owner, LiquidityRange memory range, uint256, bytes memory hookData, bool claims) internal - returns (BalanceDelta delta) + returns (BalanceDelta callerDelta) { - delta = _collect(owner, range, hookData); - _closeCallerDeltas(delta, range.poolKey.currency0, range.poolKey.currency1, owner, claims); - _closeAllDeltas(range.poolKey.currency0, range.poolKey.currency1); + BalanceDelta thisDelta; + (callerDelta, thisDelta) = _collect(owner, range, hookData); + _closeCallerDeltas(callerDelta, range.poolKey.currency0, range.poolKey.currency1, owner, claims); + _closeThisDeltas(thisDelta, range.poolKey.currency0, range.poolKey.currency1); } function _lockAndCollect(address owner, LiquidityRange memory range, bytes memory hookData, bool claims) diff --git a/contracts/libraries/Position.sol b/contracts/libraries/Position.sol index a3a099ba..11ef1771 100644 --- a/contracts/libraries/Position.sol +++ b/contracts/libraries/Position.sol @@ -14,6 +14,11 @@ library PositionLibrary { position.tokensOwed1 += uint128(tokensOwed.amount1()); } + function clearTokensOwed(IBaseLiquidityManagement.Position storage position) internal { + position.tokensOwed0 = 0; + position.tokensOwed1 = 0; + } + function addLiquidity(IBaseLiquidityManagement.Position storage position, uint256 liquidity) internal { unchecked { position.liquidity += liquidity; From f3c540d1410374aa0e0031ade3582ae0d13c6fff Mon Sep 17 00:00:00 2001 From: Sara Reynolds Date: Thu, 27 Jun 2024 18:28:04 -0400 Subject: [PATCH 3/5] update decrease/collect --- .../autocompound_exactUnclaimedFees.snap | 2 +- ...exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .../autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc20.snap | 2 +- .../decreaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .../increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- contracts/NonfungiblePositionManager.sol | 9 ++- contracts/base/BaseLiquidityManagement.sol | 30 ++++++-- .../INonfungiblePositionManager.sol | 5 +- test/position-managers/FeeCollection.t.sol | 76 +++++-------------- .../NonfungiblePositionManager.t.sol | 3 +- 13 files changed, 57 insertions(+), 82 deletions(-) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 20308207..8e881fb8 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -258574 \ No newline at end of file +258575 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index be27e821..f44837b7 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -190947 \ No newline at end of file +190948 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index 3632f8d2..81d04dab 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -279113 \ No newline at end of file +279114 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index 95f17313..461e5928 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1 +1 @@ -133893 \ No newline at end of file +177014 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index 91878896..1a5a1ce2 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1 +1 @@ -121836 \ No newline at end of file +177026 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index cee1a41b..786ac121 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -171338 \ No newline at end of file +171339 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index 677231e7..24ec8e92 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -146920 \ No newline at end of file +146921 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index 65812e8b..ee03852e 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -466627 \ No newline at end of file +466628 \ No newline at end of file diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index a461d1db..696f315d 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -86,10 +86,10 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit function decreaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims) public isAuthorizedForToken(tokenId) - returns (BalanceDelta delta) + returns (BalanceDelta delta, BalanceDelta thisDelta) { TokenPosition memory tokenPos = tokenPositions[tokenId]; - delta = _lockAndDecreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData, claims); + (delta, thisDelta) = _lockAndDecreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData, claims); } function burn(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) @@ -102,7 +102,8 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit LiquidityRangeId rangeId = tokenPosition.range.toId(); Position storage position = positions[msg.sender][rangeId]; if (0 < position.liquidity) { - delta = decreaseLiquidity(tokenId, position.liquidity, hookData, claims); + (delta,) = decreaseLiquidity(tokenId, position.liquidity, hookData, claims); + (delta) = collect(tokenId, recipient, hookData, claims); } require(position.tokensOwed0 == 0 && position.tokensOwed1 == 0, "NOT_EMPTY"); delete positions[msg.sender][rangeId]; @@ -114,7 +115,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit // TODO: in v3, we can partially collect fees, but what was the usecase here? function collect(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) - external + public returns (BalanceDelta delta) { TokenPosition memory tokenPos = tokenPositions[tokenId]; diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index d07eb1cd..e83f00fd 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -77,7 +77,9 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { if (op == LiquidityOperation.INCREASE) { return abi.encode(_increaseLiquidityAndZeroOut(owner, range, liquidityChange, hookData, claims)); } else if (op == LiquidityOperation.DECREASE) { - return abi.encode(_decreaseLiquidityAndZeroOut(owner, range, liquidityChange, hookData, claims)); + (BalanceDelta callerDelta, BalanceDelta thisDelta) = + _decreaseLiquidityAndZeroOut(owner, range, liquidityChange, hookData, claims); + return abi.encode(callerDelta, thisDelta); } else if (op == LiquidityOperation.COLLECT) { return abi.encode(_collectAndZeroOut(owner, range, 0, hookData, claims)); } else { @@ -215,8 +217,7 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { ); } - /// @dev Note that we do not update tokensOwed since all fees collected from this modify call are automatically sent to the caller. - /// Any outstanding amounts owed to the caller from tokensOwed0/tokensOwed1 must be collected explicitly. + /// Any outstanding amounts owed to the caller from the underlying modify call must be collected explicitly with `collect`. function _decreaseLiquidity( address owner, LiquidityRange memory range, @@ -235,6 +236,20 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { // Account for fees accrued to other users on the same range. (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); + BalanceDelta tokensOwed; + + // Flush the callerDelta, incrementing the tokensOwed to the user and the amount claimable to this contract. + if (callerDelta.amount0() > 0) { + (tokensOwed, callerDelta, thisDelta) = + _moveCallerDeltaToTokensOwed(true, tokensOwed, callerDelta, thisDelta); + } + + if (callerDelta.amount1() > 0) { + (tokensOwed, callerDelta, thisDelta) = + _moveCallerDeltaToTokensOwed(false, tokensOwed, callerDelta, thisDelta); + } + + position.addTokensOwed(tokensOwed); position.subtractLiquidity(liquidityToRemove); } @@ -244,9 +259,7 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { uint256 liquidityToRemove, bytes memory hookData, bool claims - ) internal returns (BalanceDelta callerDelta) { - // Todo move to transient storage, and potentially bubble up both deltas - BalanceDelta thisDelta; + ) internal returns (BalanceDelta callerDelta, BalanceDelta thisDelta) { (callerDelta, thisDelta) = _decreaseLiquidity(owner, range, liquidityToRemove, hookData); _closeCallerDeltas(callerDelta, range.poolKey.currency0, range.poolKey.currency1, owner, claims); _closeThisDeltas(thisDelta, range.poolKey.currency0, range.poolKey.currency1); @@ -258,10 +271,10 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { uint256 liquidityToRemove, bytes memory hookData, bool claims - ) internal returns (BalanceDelta) { + ) internal returns (BalanceDelta, BalanceDelta) { return abi.decode( manager.unlock(abi.encode(LiquidityOperation.DECREASE, owner, range, liquidityToRemove, hookData, claims)), - (BalanceDelta) + (BalanceDelta, BalanceDelta) ); } @@ -270,6 +283,7 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { returns (BalanceDelta callerDelta, BalanceDelta thisDelta) { // Do not add or decrease liquidity, just trigger fee updates. + // TODO: Fails when position is empty (BalanceDelta liquidityDelta, BalanceDelta totalFeesAccrued) = _modifyLiquidity(owner, range, 0, hookData); Position storage position = positions[owner][range.toId()]; diff --git a/contracts/interfaces/INonfungiblePositionManager.sol b/contracts/interfaces/INonfungiblePositionManager.sol index 6b09efe5..af011df5 100644 --- a/contracts/interfaces/INonfungiblePositionManager.sol +++ b/contracts/interfaces/INonfungiblePositionManager.sol @@ -37,10 +37,11 @@ interface INonfungiblePositionManager { /// @param liquidity The amount of liquidity to remove /// @param hookData Arbitrary data passed to the hook /// @param claims Whether the removed liquidity is sent as ERC-6909 claim tokens - /// @return delta Corresponding balance changes as a result of decreasing liquidity + /// @return delta Corresponding balance changes as a result of decreasing liquidity applied to user + /// @return thisDelta Corresponding balance changes as a result of decreasing liquidity applied to lpm function decreaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims) external - returns (BalanceDelta delta); + returns (BalanceDelta delta, BalanceDelta thisDelta); /// @notice Burn a position and delete the tokenId /// @dev It removes liquidity and collects fees if the position is not empty diff --git a/test/position-managers/FeeCollection.t.sol b/test/position-managers/FeeCollection.t.sol index 643f6303..e2753f72 100644 --- a/test/position-managers/FeeCollection.t.sol +++ b/test/position-managers/FeeCollection.t.sol @@ -216,47 +216,6 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { function test_collect_donate() public {} function test_collect_donate_sameRange() public {} - function test_decreaseLiquidity_sameRange( - IPoolManager.ModifyLiquidityParams memory params, - uint256 liquidityDeltaBob - ) public { - uint256 tokenIdAlice; - uint256 tokenIdBob; - params.liquidityDelta = bound(params.liquidityDelta, 10e18, 10_000e18); - params = createFuzzyLiquidityParams(key, params, SQRT_PRICE_1_1); - vm.assume(params.tickLower < 0 && 0 < params.tickUpper); // require two-sided liquidity - - liquidityDeltaBob = bound(liquidityDeltaBob, 100e18, 100_000e18); - - LiquidityRange memory range = - LiquidityRange({poolKey: key, tickLower: params.tickLower, tickUpper: params.tickUpper}); - vm.prank(alice); - (tokenIdAlice,) = lpm.mint(range, uint256(params.liquidityDelta), block.timestamp + 1, alice, ZERO_BYTES); - - vm.prank(bob); - (tokenIdBob,) = lpm.mint(range, liquidityDeltaBob, block.timestamp + 1, bob, ZERO_BYTES); - - // swap to create fees - uint256 swapAmount = 0.001e18; - swap(key, true, -int256(swapAmount), ZERO_BYTES); - - // alice removes all of her liquidity - vm.prank(alice); - BalanceDelta aliceDelta = lpm.decreaseLiquidity(tokenIdAlice, uint256(params.liquidityDelta), ZERO_BYTES, true); - assertEq(uint256(uint128(aliceDelta.amount0())), manager.balanceOf(alice, currency0.toId())); - assertEq(uint256(uint128(aliceDelta.amount1())), manager.balanceOf(alice, currency1.toId())); - - // bob removes half of his liquidity - vm.prank(bob); - BalanceDelta bobDelta = lpm.decreaseLiquidity(tokenIdBob, liquidityDeltaBob / 2, ZERO_BYTES, true); - assertEq(uint256(uint128(bobDelta.amount0())), manager.balanceOf(bob, currency0.toId())); - assertEq(uint256(uint128(bobDelta.amount1())), manager.balanceOf(bob, currency1.toId())); - - // position manager holds no fees now - assertApproxEqAbs(manager.balanceOf(address(lpm), currency0.toId()), 0, 1 wei); - assertApproxEqAbs(manager.balanceOf(address(lpm), currency1.toId()), 0, 1 wei); - } - /// @dev Alice and bob create liquidity on the same range /// when alice decreases liquidity, she should only collect her fees function test_decreaseLiquidity_sameRange_exact() public { @@ -281,39 +240,38 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { // alice decreases liquidity vm.prank(alice); - BalanceDelta aliceDelta = lpm.decreaseLiquidity(tokenIdAlice, liquidityAlice, ZERO_BYTES, true); + BalanceDelta aliceDelta; + BalanceDelta thisDelta; + (aliceDelta, thisDelta) = lpm.decreaseLiquidity(tokenIdAlice, liquidityAlice, ZERO_BYTES, true); uint256 tolerance = 0.000000001 ether; - // alice claims original principal + her fees + uint256 lpmBalance0 = manager.balanceOf(address(lpm), currency0.toId()); + uint256 lpmBalance1 = manager.balanceOf(address(lpm), currency1.toId()); + + // lpm collects alice's principal + all fees accrued on the range assertApproxEqAbs( - manager.balanceOf(alice, currency0.toId()), - uint256(int256(-lpDeltaAlice.amount0())) - + swapAmount.mulWadDown(FEE_WAD).mulDivDown(liquidityAlice, liquidityAlice + liquidityBob), - tolerance + lpmBalance0, uint256(int256(-lpDeltaAlice.amount0())) + swapAmount.mulWadDown(FEE_WAD), tolerance ); assertApproxEqAbs( - manager.balanceOf(alice, currency1.toId()), - uint256(int256(-lpDeltaAlice.amount1())) - + swapAmount.mulWadDown(FEE_WAD).mulDivDown(liquidityAlice, liquidityAlice + liquidityBob), - tolerance + lpmBalance1, uint256(int256(-lpDeltaAlice.amount1())) + swapAmount.mulWadDown(FEE_WAD), tolerance ); // bob decreases half of his liquidity vm.prank(bob); - BalanceDelta bobDelta = lpm.decreaseLiquidity(tokenIdBob, liquidityBob / 2, ZERO_BYTES, true); + BalanceDelta bobDelta; + (bobDelta, thisDelta) = lpm.decreaseLiquidity(tokenIdBob, liquidityBob / 2, ZERO_BYTES, true); - // bob claims half of the original principal + his fees + // lpm collects half of bobs principal + // the fee amount has already been collected with alice's calls assertApproxEqAbs( - manager.balanceOf(bob, currency0.toId()), - uint256(int256(-lpDeltaBob.amount0()) / 2) - + swapAmount.mulWadDown(FEE_WAD).mulDivDown(liquidityBob, liquidityAlice + liquidityBob), + manager.balanceOf(address(lpm), currency0.toId()) - lpmBalance0, + uint256(int256(-lpDeltaBob.amount0()) / 2), tolerance ); assertApproxEqAbs( - manager.balanceOf(bob, currency1.toId()), - uint256(int256(-lpDeltaBob.amount1()) / 2) - + swapAmount.mulWadDown(FEE_WAD).mulDivDown(liquidityBob, liquidityAlice + liquidityBob), + manager.balanceOf(address(lpm), currency1.toId()) - lpmBalance1, + uint256(int256(-lpDeltaBob.amount1()) / 2), tolerance ); } diff --git a/test/position-managers/NonfungiblePositionManager.t.sol b/test/position-managers/NonfungiblePositionManager.t.sol index c1cad0c1..3d59572b 100644 --- a/test/position-managers/NonfungiblePositionManager.t.sol +++ b/test/position-managers/NonfungiblePositionManager.t.sol @@ -247,7 +247,8 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi uint256 balance0Before = currency0.balanceOfSelf(); uint256 balance1Before = currency1.balanceOfSelf(); - BalanceDelta delta = lpm.decreaseLiquidity(tokenId, decreaseLiquidityDelta, ZERO_BYTES, false); + (BalanceDelta delta, BalanceDelta thisDelta) = + lpm.decreaseLiquidity(tokenId, decreaseLiquidityDelta, ZERO_BYTES, false); (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, uint256(params.liquidityDelta) - decreaseLiquidityDelta); From 2299f83645f5b5599446666051f5f0c60f43b72a Mon Sep 17 00:00:00 2001 From: Sara Reynolds Date: Thu, 27 Jun 2024 18:45:57 -0400 Subject: [PATCH 4/5] remove delta function --- contracts/base/BaseLiquidityManagement.sol | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index e83f00fd..2bc939f6 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -170,20 +170,6 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { if (delta1 < 0) currency1.settle(manager, address(this), uint256(int256(-delta1)), true); } - //TODO @sara deprecate when moving to _closeThisDeltas for decreaes and collect - function _closeAllDeltas(Currency currency0, Currency currency1) internal { - (BalanceDelta delta) = manager.currencyDeltas(address(this), currency0, currency1); - int128 delta0 = delta.amount0(); - int128 delta1 = delta.amount1(); - - // Mint a receipt for the tokens owed to this address. - if (delta0 > 0) currency0.take(manager, address(this), uint128(delta0), true); - if (delta1 > 0) currency1.take(manager, address(this), uint128(delta1), true); - // Burn the receipt for tokens owed to this address. - if (delta0 < 0) currency0.settle(manager, address(this), uint256(int256(-delta0)), true); - if (delta1 < 0) currency1.settle(manager, address(this), uint256(int256(-delta1)), true); - } - function _moveCallerDeltaToTokensOwed( bool useAmount0, BalanceDelta tokensOwed, From 9eaffcd87f05828075e2e9e2cd795ae7d51e841c Mon Sep 17 00:00:00 2001 From: Sara Reynolds Date: Thu, 27 Jun 2024 20:10:45 -0400 Subject: [PATCH 5/5] update burn --- contracts/NonfungiblePositionManager.sol | 7 +++++-- contracts/base/BaseLiquidityManagement.sol | 20 +++++++++++--------- test/position-managers/FeeCollection.t.sol | 2 ++ 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index 696f315d..ab1670cd 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -97,14 +97,17 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit isAuthorizedForToken(tokenId) returns (BalanceDelta delta) { + // TODO: Burn currently decreases and collects. However its done under different locks. + // Replace once we have the execute multicall. // remove liquidity TokenPosition storage tokenPosition = tokenPositions[tokenId]; LiquidityRangeId rangeId = tokenPosition.range.toId(); Position storage position = positions[msg.sender][rangeId]; - if (0 < position.liquidity) { + if (position.liquidity > 0) { (delta,) = decreaseLiquidity(tokenId, position.liquidity, hookData, claims); - (delta) = collect(tokenId, recipient, hookData, claims); } + + collect(tokenId, recipient, hookData, claims); require(position.tokensOwed0 == 0 && position.tokensOwed1 == 0, "NOT_EMPTY"); delete positions[msg.sender][rangeId]; delete tokenPositions[tokenId]; diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index 2bc939f6..df54345a 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -268,18 +268,20 @@ contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallback { internal returns (BalanceDelta callerDelta, BalanceDelta thisDelta) { - // Do not add or decrease liquidity, just trigger fee updates. - // TODO: Fails when position is empty - (BalanceDelta liquidityDelta, BalanceDelta totalFeesAccrued) = _modifyLiquidity(owner, range, 0, hookData); - Position storage position = positions[owner][range.toId()]; - // Also updates the position's the feeGrowthInsideLast variables in storage. - (BalanceDelta callerFeesAccrued) = _updateFeeGrowth(range, position); + // Only call modify if there is still liquidty in this position. + if (position.liquidity != 0) { + // Do not add or decrease liquidity, just trigger fee updates. + (BalanceDelta liquidityDelta, BalanceDelta totalFeesAccrued) = _modifyLiquidity(owner, range, 0, hookData); - // Account for fees accrued to other users on the same range. - // TODO: Opt when liquidityDelta == 0 - (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); + // Also updates the position's the feeGrowthInsideLast variables in storage. + (BalanceDelta callerFeesAccrued) = _updateFeeGrowth(range, position); + + // Account for fees accrued to other users on the same range. + // TODO: Opt when liquidityDelta == 0 + (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); + } // Allow the caller to collect the tokens owed. // Tokens owed that the caller collects is paid for by this contract. diff --git a/test/position-managers/FeeCollection.t.sol b/test/position-managers/FeeCollection.t.sol index e2753f72..e89ff68a 100644 --- a/test/position-managers/FeeCollection.t.sol +++ b/test/position-managers/FeeCollection.t.sol @@ -218,6 +218,8 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { /// @dev Alice and bob create liquidity on the same range /// when alice decreases liquidity, she should only collect her fees + /// TODO Add back fuzz test on liquidityDeltaBob + /// TODO Assert state changes for lpm balance, position state, and return values function test_decreaseLiquidity_sameRange_exact() public { // alice and bob create liquidity on the same range [-120, 120] LiquidityRange memory range = LiquidityRange({poolKey: key, tickLower: -120, tickUpper: 120});