fix: adjust per comments

pancakeswap · Nov 13, 2024 · 91ed183 · 91ed183
1 parent e7bf244
commit 91ed183
Show file tree

Hide file tree

Showing 14 changed files with 94 additions and 44 deletions.
diff --git a/.forge-snapshots/BinHookTest#testBurnSucceedsWithHook.snap b/.forge-snapshots/BinHookTest#testBurnSucceedsWithHook.snap
@@ -1 +1 @@
-179611
+233082
diff --git a/.forge-snapshots/BinMintBurnFeeHookTest#test_Burn.snap b/.forge-snapshots/BinMintBurnFeeHookTest#test_Burn.snap
@@ -1 +1 @@
-167865
+221336
diff --git a/.forge-snapshots/BinPoolManagerBytecodeSize.snap b/.forge-snapshots/BinPoolManagerBytecodeSize.snap
@@ -1 +1 @@
-23766
+23636
diff --git a/.forge-snapshots/BinPoolManagerTest#testBurnNativeCurrency.snap b/.forge-snapshots/BinPoolManagerTest#testBurnNativeCurrency.snap
@@ -1 +1 @@
-135054
+183431
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasBurnHalfBin.snap b/.forge-snapshots/BinPoolManagerTest#testGasBurnHalfBin.snap
@@ -1 +1 @@
-143286
+143280
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasBurnNineBins.snap b/.forge-snapshots/BinPoolManagerTest#testGasBurnNineBins.snap
@@ -1 +1 @@
-297444
+330058
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap b/.forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap
@@ -1 +1 @@
-128227
+174897
diff --git a/src/pool-bin/libraries/BinPool.sol b/src/pool-bin/libraries/BinPool.sol
@@ -317,21 +317,26 @@ library BinPool {
             bytes32 binReserves = self.reserveOfBin[id];
             uint256 supply = self.shareOfBin[id];
 
+            /// @notice if user is last lp for this bin, amountToBurn would be amountToBurn + MINIMUM_SHARE
             amountToBurn = _subShare(self, params.from, id, params.salt, amountToBurn);
 
-            bytes32 amountsOutFromBin = binReserves.getAmountOutOfBin(amountToBurn, supply);
-            if (supply == amountToBurn) feeAmountToProtocol = binReserves.getAmountOutOfBin(MINIMUM_SHARE, supply);
-
-            if (amountsOutFromBin == 0) revert BinPool__ZeroAmountsOut(id);
-
-            binReserves = binReserves.sub(amountsOutFromBin);
-
+            bytes32 amountsOutFromBin;
+            /// @dev if user is last lp for this bin, supply == amountToBurn == amountsToBurn[i] + MINIMUM_SHARE
+            /// then we will remove all the liqudity from the bin where MINIMUM_SHARE will be withdrawn as protocol fee
+            /// to prevent MINIMUM_SHARE from being locked up in the bin which cause extra gas cost when swap
             if (supply == amountToBurn) {
                 _removeBinIdToTree(self, id);
 
                 /// @notice withdraw all the liquidity from the bin, the locked up share will be withdrawn as protocol fee
-                amountsOutFromBin.sub(feeAmountToProtocol);
+                /// @dev we can't use "binReserves.getAmountOutOfBin(MINIMUM_SHARE, supply)" to calc the fee as it will round down to 0
+                amountsOutFromBin = binReserves.getAmountOutOfBin(amountToBurn - MINIMUM_SHARE, supply);
+                feeAmountToProtocol = feeAmountToProtocol.add(binReserves.sub(amountsOutFromBin));
+                binReserves = 0;
+            } else {
+                amountsOutFromBin = binReserves.getAmountOutOfBin(amountToBurn, supply);
+                binReserves = binReserves.sub(amountsOutFromBin);
             }
+            if (amountsOutFromBin == 0) revert BinPool__ZeroAmountsOut(id);
 
             self.reserveOfBin[id] = binReserves;
             amounts[i] = amountsOutFromBin;
@@ -471,6 +476,8 @@ library BinPool {
     }
 
     /// @notice Subtract share from user's position and update total share supply of bin
+    /// @param shares - amount of share to deduct specified by user
+    /// @return amountsToBurn amount of share burned, nornally equals to shares except when bin is left with MINIMUM_SHARE, amountsToBurn will be share + MINIMUM_SHARE
     function _subShare(State storage self, address owner, uint24 binId, bytes32 salt, uint256 shares)
         internal
         returns (uint256 amountsToBurn)

diff --git a/test/pool-bin/BinHookReturnsDelta.t.sol b/test/pool-bin/BinHookReturnsDelta.t.sol
@@ -127,11 +127,15 @@ contract BinHookReturnsDelta is Test, GasSnapshot, BinTestHelper {
         binLiquidityHelper.burn(key, burnParams, "");
 
         (uint128 reserveXAfter, uint128 reserveYAfter,,) = poolManager.getBin(key.toId(), activeId);
-        // reserve non zero due to min liquidity locked up in the bin
+        // reserve back to zero due to min liquidity locked up in the bin will be withdrawn as protocol fee
         assertEq(reserveXAfter, 0);
         assertEq(reserveYAfter, 0);
-        assertEq(token0.balanceOf(address(binReturnsDeltaHook)), 0.1 ether);
-        assertEq(token1.balanceOf(address(binReturnsDeltaHook)), 0.1 ether);
+        assertEq(token0.balanceOf(address(binReturnsDeltaHook)), 0.1 ether - 1);
+        assertEq(token1.balanceOf(address(binReturnsDeltaHook)), 0.1 ether - 1);
+
+        // check protocol fee increase
+        assertEq(poolManager.protocolFeesAccrued(key.currency0), 1);
+        assertEq(poolManager.protocolFeesAccrued(key.currency1), 1);
     }
 
     function testSwap_noSwap_specifyInput() external {

diff --git a/test/pool-bin/BinMintBurnFeeHook.t.sol b/test/pool-bin/BinMintBurnFeeHook.t.sol
@@ -104,7 +104,7 @@ contract BinMintBurnFeeHookTest is Test, GasSnapshot, BinTestHelper {
         token1.mint(address(this), 10 ether);
 
         IBinPoolManager.MintParams memory mintParams = _getSingleBinMintParams(activeId, 1 ether, 1 ether);
-        BalanceDelta delta = binLiquidityHelper.mint(key, mintParams, abi.encode(0));
+        binLiquidityHelper.mint(key, mintParams, abi.encode(0));
 
         assertEq(token0.balanceOf(address(this)), 7 ether);
         assertEq(token1.balanceOf(address(this)), 7 ether);
@@ -120,15 +120,18 @@ contract BinMintBurnFeeHookTest is Test, GasSnapshot, BinTestHelper {
         binLiquidityHelper.burn(key, burnParams, "");
         snapEnd();
 
-        // +1 from remove liqudiity, -4 from hook fee
-        assertEq(token0.balanceOf(address(this)), 7 ether + 1 ether - 4 ether);
-        assertEq(token1.balanceOf(address(this)), 7 ether + 1 ether - 4 ether);
+        // +1 ether from remove liqudiity, -4 ether from hook fee
+        // +3 from min_liquidity amount as -1 (min_liquidity) + 1 * 4 (fee)
+        assertEq(token0.balanceOf(address(this)), 7 ether + 1 ether - 4 ether + 3);
+        assertEq(token1.balanceOf(address(this)), 7 ether + 1 ether - 4 ether + 3);
+
+        // -1 ether from remove liquidity, +4 ether from hook calling vault.mint
+        // -3 as min_liquidity amount as 1 (min_liquidity) still in vault but hook's fee is less 1 * 4 (fee)
+        assertEq(token0.balanceOf(address(vault)), 3 ether - 1 ether + 4 ether - 3);
+        assertEq(token1.balanceOf(address(vault)), 3 ether - 1 ether + 4 ether - 3);
 
-        // -1 from remove liquidity, +4 from hook calling vault.mint
-        assertEq(token0.balanceOf(address(vault)), 3 ether - 1 ether + 4 ether);
-        assertEq(token1.balanceOf(address(vault)), 3 ether - 1 ether + 4 ether);
-        assertEq(vault.balanceOf(address(binMintBurnFeeHook), key.currency0), 2 ether + 4 ether);
-        assertEq(vault.balanceOf(address(binMintBurnFeeHook), key.currency1), 2 ether + 4 ether);
+        assertEq(vault.balanceOf(address(binMintBurnFeeHook), key.currency0), 2 ether + 4 ether - 4);
+        assertEq(vault.balanceOf(address(binMintBurnFeeHook), key.currency1), 2 ether + 4 ether - 4);
     }
 
     receive() external payable {}

diff --git a/test/pool-bin/BinPoolManager.t.sol b/test/pool-bin/BinPoolManager.t.sol
@@ -641,7 +641,8 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         uint256[] memory ids = new uint256[](1);
         bytes32[] memory amounts = new bytes32[](1);
         ids[0] = activeId;
-        amounts[0] = 0x00000000000000000de0b6b3a764000000000000000000000de0b6b3a7640000; // <wip> uint128(1e18).encode(uint128(1e18));
+        // lock-up 1e3 share are not burned by user but instead withdrawn as protocol fee
+        amounts[0] = uint128(1e18 - 1).encode(uint128(1e18 - 1));
         vm.expectEmit();
         emit IBinPoolManager.Burn(key.toId(), address(binLiquidityHelper), ids, 0, amounts);
 
@@ -710,7 +711,8 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         uint256[] memory ids = new uint256[](1);
         bytes32[] memory amounts = new bytes32[](1);
         ids[0] = activeId;
-        amounts[0] = 0x00000000000000000de0b6b3a764000000000000000000000de0b6b3a7640000; // <wip> to fill in numbers uint128(1e18).encode(uint128(1e18));
+        // lock-up 1e3 share are not burned by user but instead withdrawn as protocol fee
+        amounts[0] = uint128(1e18 - 1).encode(uint128(1e18 - 1));
         vm.expectEmit();
         emit IBinPoolManager.Burn(key.toId(), address(binLiquidityHelper), ids, 0, amounts);
 

diff --git a/test/pool-bin/libraries/BinPoolDonate.t.sol b/test/pool-bin/libraries/BinPoolDonate.t.sol
@@ -87,9 +87,11 @@ contract BinPoolDonateTest is BinTestHelper {
         // Verify reserve before donate
         uint128 reserveX;
         uint128 reserveY;
-        (reserveX, reserveY,,) = poolManager.getBin(poolId, activeId);
+        uint256 totalSupply;
+        (reserveX, reserveY,, totalSupply) = poolManager.getBin(poolId, activeId);
         assertEq(reserveX, 2e18);
         assertEq(reserveY, 2e18);
+        assertEq(totalSupply, aliceShare + bobShare + BinPool.MINIMUM_SHARE);
 
         // Donate
         poolManager.donate(key, 2e18, 2e18, "");
@@ -106,14 +108,18 @@ contract BinPoolDonateTest is BinTestHelper {
 
         // lesser than 2e18 as alice is the first lp provider and liquidity locked up
         BalanceDelta removeDelta2 = removeLiquidityFromBin(key, poolManager, alice, activeId, aliceShare, "");
-        assertEq(removeDelta2.amount0(), 2e18);
-        assertEq(removeDelta2.amount1(), 2e18);
+        assertEq(removeDelta2.amount0(), 2e18 - 1);
+        assertEq(removeDelta2.amount1(), 2e18 - 1);
 
-        // Verify only min_liquidity worth of token locked up
-        (reserveX, reserveY,,) = poolManager.getBin(poolId, activeId);
+        // can see the reserve is back to 0 after all liquidity has been removed
+        (reserveX, reserveY,, totalSupply) = poolManager.getBin(poolId, activeId);
         assertEq(reserveX, 0);
         assertEq(reserveY, 0);
+        assertEq(totalSupply, 0);
+        assertEq(poolManager.protocolFeesAccrued(key.currency0), 1);
+        assertEq(poolManager.protocolFeesAccrued(key.currency1), 1);
 
+        // Verify min_liquidity worth of token locked up is accumulated to protocol fee
         vm.expectRevert(abi.encodeWithSelector(IBinPoolManager.InsufficientBinShareForDonate.selector, 0));
         poolManager.donate(key, 1e18, 1e18, "");
     }

diff --git a/test/pool-bin/libraries/BinPoolFee.t.sol b/test/pool-bin/libraries/BinPoolFee.t.sol
@@ -325,7 +325,7 @@ contract BinPoolFeeTest is BinTestHelper {
         addLiquidityToBin(_key, poolManager, bob, activeId, amountX, amountY, 1e18, 1e18, "");
     }
 
-    function test_Burn_NoFee() external {
+    function test_Burn_MinimumShareAsFee() external {
         // add liqudiity
         uint24 activeId = ID_ONE; // where token price are the same
         _addLiquidityForBurnTest(activeId, key);
@@ -337,9 +337,29 @@ contract BinPoolFeeTest is BinTestHelper {
         ids[0] = activeId;
         removeLiquidity(key, poolManager, bob, ids, balances);
 
-        // check fee. no hook for pool, so can skip check
-        assertEq(poolManager.protocolFeesAccrued(currency0), 0, "test_Burn_NoFee::1");
-        assertEq(poolManager.protocolFeesAccrued(currency1), 0, "test_Burn_NoFee::1");
+        // check fee as minimum share will be withdrawn as fee
+        // no hook for pool, so can skip check
+        assertEq(poolManager.protocolFeesAccrued(currency0), 1, "test_Burn_MinimumShareAsFee::1");
+        assertEq(poolManager.protocolFeesAccrued(currency1), 1, "test_Burn_MinimumShareAsFee::1");
+    }
+
+    function test_Burn_NoMinimumShareAsFee() external {
+        // add liqudiity
+        uint24 activeId = ID_ONE; // where token price are the same
+        _addLiquidityForBurnTest(activeId, key);
+
+        // then remove liquidity
+        uint256[] memory balances = new uint256[](1);
+        uint256[] memory ids = new uint256[](1);
+        // remove 1 share less than what was minted, so that MINIMUM_SHARE is not withdrawn as fee
+        balances[0] = poolManager.getPosition(poolId, bob, activeId, 0).share - 1;
+        ids[0] = activeId;
+        removeLiquidity(key, poolManager, bob, ids, balances);
+
+        // check fee as minimum share will not be withdrawn as fee as there are still 1 user's share left
+        // no hook for pool, so can skip check
+        assertEq(poolManager.protocolFeesAccrued(currency0), 0, "test_Burn_NoMinimumShareAsFee::1");
+        assertEq(poolManager.protocolFeesAccrued(currency1), 0, "test_Burn_NoMinimumShareAsFee::1");
     }
 
     function test_Swap_NoFee() external {

diff --git a/test/pool-bin/libraries/BinPoolLiquidity.t.sol b/test/pool-bin/libraries/BinPoolLiquidity.t.sol
@@ -192,8 +192,8 @@ contract BinPoolLiquidityTest is BinTestHelper {
         uint256 aliceBal = poolManager.getPosition(key.toId(), alice, activeId, 0).share;
         BalanceDelta aliceDelta = removeLiquidityFromBin(key, poolManager, alice, activeId, aliceBal, "");
         // -1 tokenX and -1 tokenY as they are locked as min liquidity
-        assertEq(aliceDelta.amount0() + bobDelta.amount0(), 200 ether);
-        assertEq(aliceDelta.amount1() + bobDelta.amount1(), 1 ether);
+        assertEq(aliceDelta.amount0() + bobDelta.amount0(), 200 ether - 1);
+        assertEq(aliceDelta.amount1() + bobDelta.amount1(), 1 ether - 1);
     }
 
     function test_MintWithDifferentBins() external {
@@ -317,7 +317,7 @@ contract BinPoolLiquidityTest is BinTestHelper {
         removeLiquidity(key, poolManager, bob, ids, balances);
 
         // 6 as liquidity was added to 6 bins, each bin lock 1 token due to min share
-        uint256 lockedTokenDueToMinShare = 0;
+        uint256 lockedTokenDueToMinShare = 6;
         {
             // balanceDelta positive (so user need to call take/mint)
             uint256 balanceDelta0 = uint128(vault.balanceDeltaOfPool(poolId).amount0());
@@ -328,8 +328,12 @@ contract BinPoolLiquidityTest is BinTestHelper {
 
         reserveX = vault.reservesOfApp(address(key.poolManager), key.currency0);
         reserveY = vault.reservesOfApp(address(key.poolManager), key.currency1);
-        assertEq(reserveX, lockedTokenDueToMinShare, "test_BurnPartial::3");
-        assertEq(reserveY, lockedTokenDueToMinShare, "test_BurnPartial::4");
+        assertEq(reserveX, lockedTokenDueToMinShare, "test_SimpleBurn::3");
+        assertEq(reserveY, lockedTokenDueToMinShare, "test_SimpleBurn::4");
+
+        // make sure all those locked up tokens are now protocol fee
+        assertEq(poolManager.protocolFeesAccrued(key.currency0), lockedTokenDueToMinShare, "test_SimpleBurn::5");
+        assertEq(poolManager.protocolFeesAccrued(key.currency1), lockedTokenDueToMinShare, "test_SimpleBurn::6");
     }
 
     function test_BurnHalfTwice() external {
@@ -376,11 +380,15 @@ contract BinPoolLiquidityTest is BinTestHelper {
         removeLiquidity(key, poolManager, bob, ids, halfbalances);
 
         // 6 as liquidity was added to 6 bins, each bin lock 1 token due to min share
-        uint256 lockedTokenDueToMinShare = 0;
+        uint256 lockedTokenDueToMinShare = 6;
         reserveX = vault.reservesOfApp(address(key.poolManager), key.currency0); // vault.reservesOfPool(poolId, 0);
         reserveY = vault.reservesOfApp(address(key.poolManager), key.currency1); // vault.reservesOfPool(poolId, 1);
         assertEq(reserveX, lockedTokenDueToMinShare, "test_BurnPartial::5");
         assertEq(reserveY, lockedTokenDueToMinShare, "test_BurnPartial::6");
+
+        // make sure all those locked up tokens are now protocol fee
+        assertEq(poolManager.protocolFeesAccrued(key.currency0), lockedTokenDueToMinShare, "test_BurnPartial::7");
+        assertEq(poolManager.protocolFeesAccrued(key.currency1), lockedTokenDueToMinShare, "test_BurnPartial::8");
     }
 
     function test_revert_BurnEmptyArraysOrDifferent() external {