feat: burn 1e3 shares for first mint into BinPool (#212)

* feat: example of option 2 * feat: fix all existing test cases with min share burned * test: fix remaining tests * feat: clean up more test * test: format test code * feat: include updated gas cost * feat: update IBinPoolManager doc per PR * feat: burn 1e3 shares for first mint into BinPool - part 2 (#215) * feat: example if we tweak add/remove bin to tree * feat: updated gas cost * feat: add test around getNextNonEmptyBin * test: updated forge gas snapshot
pancakeswap · Nov 14, 2024 · 1b4b44e · 1b4b44e
1 parent c1e4f95
commit 1b4b44e
Show file tree

Hide file tree

Showing 23 changed files with 199 additions and 75 deletions.
diff --git a/.forge-snapshots/BinHookTest#testBurnSucceedsWithHook.snap b/.forge-snapshots/BinHookTest#testBurnSucceedsWithHook.snap
@@ -1 +1 @@
-178130
+197607
diff --git a/.forge-snapshots/BinHookTest#testMintSucceedsWithHook.snap b/.forge-snapshots/BinHookTest#testMintSucceedsWithHook.snap
@@ -1 +1 @@
-311495
+311821
diff --git a/.forge-snapshots/BinMintBurnFeeHookTest#test_Burn.snap b/.forge-snapshots/BinMintBurnFeeHookTest#test_Burn.snap
@@ -1 +1 @@
-170145
+185889
diff --git a/.forge-snapshots/BinMintBurnFeeHookTest#test_Mint.snap b/.forge-snapshots/BinMintBurnFeeHookTest#test_Mint.snap
@@ -1 +1 @@
-410577
+410773
diff --git a/.forge-snapshots/BinPoolManagerBytecodeSize.snap b/.forge-snapshots/BinPoolManagerBytecodeSize.snap
@@ -1 +1 @@
-23558
+23675
diff --git a/.forge-snapshots/BinPoolManagerTest#testBurnNativeCurrency.snap b/.forge-snapshots/BinPoolManagerTest#testBurnNativeCurrency.snap
@@ -1 +1 @@
-133892
+147984
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasBurnHalfBin.snap b/.forge-snapshots/BinPoolManagerTest#testGasBurnHalfBin.snap
@@ -1 +1 @@
-142717
+142994
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasBurnNineBins.snap b/.forge-snapshots/BinPoolManagerTest#testGasBurnNineBins.snap
@@ -1 +1 @@
-289683
+295835
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap b/.forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap
@@ -1 +1 @@
-127065
+139450
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-1.snap b/.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-1.snap
@@ -1 +1 @@
-970284
+973186
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-2.snap b/.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-2.snap
@@ -1 +1 @@
-329605
+331940
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-1.snap b/.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-1.snap
@@ -1 +1 @@
-337752
+338078
diff --git a/.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-2.snap b/.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-2.snap
@@ -1 +1 @@
-140304
+140567
diff --git a/.forge-snapshots/BinPoolManagerTest#testMintNativeCurrency.snap b/.forge-snapshots/BinPoolManagerTest#testMintNativeCurrency.snap
@@ -1 +1 @@
-304791
+305117
diff --git a/src/pool-bin/interfaces/IBinPoolManager.sol b/src/pool-bin/interfaces/IBinPoolManager.sol
@@ -170,6 +170,8 @@ interface IBinPoolManager is IProtocolFees, IPoolManager, IExtsload {
     function initialize(PoolKey memory key, uint24 activeId) external;
 
     /// @notice Add liquidity to a pool
+    /// @dev For the first liquidity added to a bin, the share minted would be slightly lessser (1e3 lesser) to prevent
+    /// share inflation attack.
     /// @return delta BalanceDelta, will be negative indicating how much total amt0 and amt1 liquidity added
     /// @return mintArray Liquidity added in which ids, how much amt0, amt1 and how much liquidity added
     function mint(PoolKey memory key, IBinPoolManager.MintParams calldata params, bytes calldata hookData)

diff --git a/src/pool-bin/libraries/BinPool.sol b/src/pool-bin/libraries/BinPool.sol
@@ -67,6 +67,10 @@ library BinPool {
         mapping(bytes32 => bytes32) level2;
     }
 
+    /// @dev when a bin has supply for the first time, 1e3 share will be locked up
+    /// this is to prevent share inflation attack on BinPool type
+    uint256 constant MINIMUM_SHARE = 1e3;
+
     function initialize(State storage self, uint24 activeId, uint24 protocolFee, uint24 lpFee) internal {
         /// An initialized pool will not have activeId: 0
         if (self.slot0.activeId() != 0) revert PoolAlreadyInitialized();
@@ -330,7 +334,8 @@ library BinPool {
 
             binReserves = binReserves.sub(amountsOutFromBin);
 
-            if (supply == amountToBurn) _removeBinIdToTree(self, id);
+            /// @dev _removeBinIdToTree if supply is MINIMUM_SHARE after burning as min share is too low liquidity for trade anyway
+            if (supply - amountToBurn == MINIMUM_SHARE) _removeBinIdToTree(self, id);
 
             self.reserveOfBin[id] = binReserves;
             amounts[i] = amountsOutFromBin;
@@ -397,12 +402,12 @@ library BinPool {
             amountsLeft = amountsLeft.sub(amountsIn);
             feeAmountToProtocol = feeAmountToProtocol.add(binFeeAmt);
 
+            shares = _addShare(self, params.to, id, params.salt, shares);
+
             arrays.ids[i] = id;
             arrays.amounts[i] = amountsInToBin;
             arrays.liquidityMinted[i] = shares;
 
-            _addShare(self, params.to, id, params.salt, shares);
-
             compositionFeeAmount = compositionFeeAmount.add(binCompositionFee);
 
             unchecked {
@@ -467,7 +472,8 @@ library BinPool {
         }
 
         if (shares == 0 || amountsInToBin == 0) revert BinPool__ZeroShares(id);
-        if (supply == 0) _addBinIdToTree(self, id);
+        /// @dev if supply was originally MINIMUM_SHARE (people added and remove liquidity before) or 0 (new bin), add binId to tree
+        if (supply <= MINIMUM_SHARE) _addBinIdToTree(self, id);
 
         bytes32 newReserves = binReserves.add(amountsInToBin);
         if (newReserves.getLiquidity(price) > Constants.MAX_LIQUIDITY_PER_BIN) {
@@ -484,8 +490,20 @@ library BinPool {
     }
 
     /// @notice Add share to user's position and update total share supply of bin
-    function _addShare(State storage self, address owner, uint24 binId, bytes32 salt, uint256 shares) internal {
-        self.positions.get(owner, binId, salt).addShare(shares);
+    /// @dev if bin is empty, deduct MINIMUM_SHARE from shares
+    /// @return userShareAdded The amount of share added to user's position
+    function _addShare(State storage self, address owner, uint24 binId, bytes32 salt, uint256 shares)
+        internal
+        returns (uint256 userShareAdded)
+    {
+        userShareAdded = shares;
+        if (self.shareOfBin[binId] == 0) {
+            /// @dev Only for first liquidity provider for the bin, deduct MINIMUM_SHARE, expected to underflow
+            /// if shares < MINIMUM_SHARE for first mint
+            userShareAdded = shares - MINIMUM_SHARE;
+        }
+
+        self.positions.get(owner, binId, salt).addShare(userShareAdded);
         self.shareOfBin[binId] += shares;
     }
 

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

diff --git a/test/pool-bin/BinMintBurnFeeHook.t.sol b/test/pool-bin/BinMintBurnFeeHook.t.sol
@@ -113,21 +113,25 @@ contract BinMintBurnFeeHookTest is Test, GasSnapshot, BinTestHelper {
         assertEq(vault.balanceOf(address(binMintBurnFeeHook), key.currency0), 2 ether);
         assertEq(vault.balanceOf(address(binMintBurnFeeHook), key.currency1), 2 ether);
 
+        // take 4x the burn amount as fee
         IBinPoolManager.BurnParams memory burnParams =
             _getSingleBinBurnLiquidityParams(key, poolManager, activeId, address(binLiquidityHelper), 100);
         snapStart("BinMintBurnFeeHookTest#test_Burn");
         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 eth from remove liqudiity, -4 eth 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 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);
+        // -1 eth from remove liquidity, +4 eth from hook calling vault.mint
+        assertEq(token0.balanceOf(address(vault)), 3 ether - 1 ether + 4 ether - 3);
+        assertEq(token1.balanceOf(address(vault)), 3 ether - 1 ether + 4 ether - 3);
+
+        // -4 as due to min_liquidity = 1, hook took 4 token less fee
+        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
@@ -458,9 +458,17 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         for (uint256 i = 0; i < binIds.length; i++) {
             (uint128 binReserveX, uint128 binReserveY,,) = poolManager.getBin(key.toId(), binIds[i]);
 
-            // make sure the liquidity is added to the correct bin
-            assertEq(binReserveX, 0 ether);
-            assertEq(binReserveY, 0 ether);
+            // should have 1 token left due to min liquidity
+            if (binIds[i] < activeId) {
+                assertEq(binReserveX, 0);
+                assertEq(binReserveY, 1);
+            } else if (binIds[i] > activeId) {
+                assertEq(binReserveX, 1);
+                assertEq(binReserveY, 0);
+            } else {
+                assertEq(binReserveX, 1);
+                assertEq(binReserveY, 1);
+            }
 
             BinPosition.Info memory position =
                 poolManager.getPosition(key.toId(), address(binLiquidityHelper), binIds[i], salt);
@@ -480,6 +488,7 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         token0.mint(address(this), 30 ether);
         token1.mint(address(this), 30 ether);
 
+        // mint for salt1
         (IBinPoolManager.MintParams memory mintParams, uint24[] memory binIds) =
             _getMultipleBinMintParams(activeId, 2 ether, 2 ether, 5, 5, salt1);
         binLiquidityHelper.mint(key, mintParams, "");
@@ -514,6 +523,7 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         }
 
         {
+            // now mint for salt2
             (mintParams, binIds) = _getMultipleBinMintParams(activeId, 2 ether, 2 ether, 5, 5, salt2);
             binLiquidityHelper.mint(key, mintParams, "");
 
@@ -542,11 +552,13 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
                 // only position with salt 0 should be empty
                 assertTrue(position0.share == 0);
                 assertTrue(position1.share != 0);
-                assertTrue(position1.share == position2.share);
+                // // 1e3 is MINIMUM_SHARE locked when added liquidity first
+                assertTrue(position1.share + 1e3 == position2.share);
             }
         }
 
         {
+            // now mint for salt0
             (mintParams, binIds) = _getMultipleBinMintParams(activeId, 2 ether, 2 ether, 5, 5, salt0);
             binLiquidityHelper.mint(key, mintParams, "");
 
@@ -572,9 +584,10 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
                 BinPosition.Info memory position2 =
                     poolManager.getPosition(key.toId(), address(binLiquidityHelper), binIds[i], salt2);
 
+                // 1e3 is MINIMUM_SHARE locked when added liquidity first
                 assertTrue(position0.share != 0);
-                assertTrue(position1.share == position0.share);
-                assertTrue(position1.share == position2.share);
+                assertTrue(position1.share + 1e3 == position0.share);
+                assertTrue(position1.share + 1e3 == position2.share);
             }
         }
 
@@ -589,13 +602,13 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
             // make sure the liquidity is added to the correct bin
             if (binIds[i] < activeId) {
                 assertEq(binReserveX, 0 ether);
-                assertEq(binReserveY, 0.4 ether * 2);
+                assertEq(binReserveY, 0.4 ether * 2 + 1);
             } else if (binIds[i] > activeId) {
-                assertEq(binReserveX, 0.4 ether * 2);
+                assertEq(binReserveX, 0.4 ether * 2 + 1);
                 assertEq(binReserveY, 0 ether);
             } else {
-                assertEq(binReserveX, 0.4 ether * 2);
-                assertEq(binReserveY, 0.4 ether * 2);
+                assertEq(binReserveX, 0.4 ether * 2 + 1);
+                assertEq(binReserveY, 0.4 ether * 2 + 1);
             }
 
             BinPosition.Info memory position0 =
@@ -628,7 +641,7 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         uint256[] memory ids = new uint256[](1);
         bytes32[] memory amounts = new bytes32[](1);
         ids[0] = activeId;
-        amounts[0] = uint128(1e18).encode(uint128(1e18));
+        amounts[0] = uint128(1e18 - 1).encode(uint128(1e18 - 1)); // -1 due to minshare locked up
         vm.expectEmit();
         emit IBinPoolManager.Burn(key.toId(), address(binLiquidityHelper), ids, 0, amounts);
 
@@ -697,7 +710,7 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         uint256[] memory ids = new uint256[](1);
         bytes32[] memory amounts = new bytes32[](1);
         ids[0] = activeId;
-        amounts[0] = uint128(1e18).encode(uint128(1e18));
+        amounts[0] = uint128(1e18 - 1).encode(uint128(1e18 - 1)); // -1 due to minshare locked up
         vm.expectEmit();
         emit IBinPoolManager.Burn(key.toId(), address(binLiquidityHelper), ids, 0, amounts);
 
@@ -1181,6 +1194,69 @@ contract BinPoolManagerTest is Test, GasSnapshot, BinTestHelper {
         assertEq(shares, liquidity);
     }
 
+    function test_getNextNonEmptyBin() public {
+        poolManager.initialize(key, activeId);
+
+        // add 1 eth of tokenX and 1 eth to activeId - 2 to active + 2 bins
+        token0.mint(address(this), 10 ether);
+        token1.mint(address(this), 10 ether);
+        (IBinPoolManager.MintParams memory mintParams,) = _getMultipleBinMintParams(activeId, 2 ether, 2 ether, 3, 3);
+        binLiquidityHelper.mint(key, mintParams, "");
+
+        // swapForY is true, means search for bin to the left as tokenY reside on the left side of the bin
+        bool swapForY = true;
+        for (uint24 i = 0; i < 5; i++) {
+            // [-2, -1, activeId, 1, 2] are bins initialized due to liqudiity adding above
+            uint24 nextEmptyBin = poolManager.getNextNonEmptyBin(key.toId(), swapForY, activeId + 3 - i);
+            assertEq(nextEmptyBin, activeId + 2 - i);
+        }
+
+        IBinPoolManager.BurnParams memory burnParams;
+
+        // burn activeId-1
+        burnParams = _getSingleBinBurnLiquidityParams(key, poolManager, activeId - 1, address(binLiquidityHelper), 100);
+        binLiquidityHelper.burn(key, burnParams, "");
+
+        // as activeId-1 bin is empty now, verify the next non empty bin to the left of activeId is activeId-2
+        assertEq(poolManager.getNextNonEmptyBin(key.toId(), true, activeId), activeId - 2);
+
+        // burn activeId+1
+        burnParams = _getSingleBinBurnLiquidityParams(key, poolManager, activeId + 1, address(binLiquidityHelper), 100);
+        binLiquidityHelper.burn(key, burnParams, "");
+
+        // as activeId+1 bin is empty now, verify the next non empty bin to the left of activeId+2 is activeId
+        assertEq(poolManager.getNextNonEmptyBin(key.toId(), true, activeId + 2), activeId);
+    }
+
+    function test_getNextNonEmptyBin_AddRemoveAddLiquidity() public {
+        // initialize
+        poolManager.initialize(key, activeId);
+
+        // mint, verify activeId is in treeMath
+        token0.mint(address(this), 2 ether);
+        token1.mint(address(this), 2 ether);
+        IBinPoolManager.MintParams memory mintParams = _getSingleBinMintParams(activeId, 1 ether, 1 ether);
+        binLiquidityHelper.mint(key, mintParams, "");
+        assertEq(poolManager.getNextNonEmptyBin(key.toId(), true, activeId + 1), activeId);
+
+        // remove, verify activeId not in treeMath
+        IBinPoolManager.BurnParams memory burnParams;
+        burnParams = _getSingleBinBurnLiquidityParams(key, poolManager, activeId, address(binLiquidityHelper), 100);
+        binLiquidityHelper.burn(key, burnParams, "");
+        assertEq(poolManager.getNextNonEmptyBin(key.toId(), true, activeId + 1), type(uint24).max);
+
+        // mint, verify activeId in treeMath again
+        binLiquidityHelper.mint(key, mintParams, "");
+        assertEq(poolManager.getNextNonEmptyBin(key.toId(), true, activeId + 1), activeId);
+    }
+
+    function test_getNextNonEmptyBin_NoBinWithLiqudiity() public {
+        poolManager.initialize(key, activeId);
+
+        assertEq(poolManager.getNextNonEmptyBin(key.toId(), true, activeId), type(uint24).max);
+        assertEq(poolManager.getNextNonEmptyBin(key.toId(), false, activeId), 0);
+    }
+
     receive() external payable {}
 
     function supportsInterface(bytes4) external pure returns (bool) {