From f3c540d1410374aa0e0031ade3582ae0d13c6fff Mon Sep 17 00:00:00 2001 From: Sara Reynolds Date: Thu, 27 Jun 2024 18:28:04 -0400 Subject: [PATCH] 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);