V4 CLMigrator #51

chefburger · 2024-07-02T10:22:29Z

The new migrator for cl-pool, support the following source pools:

pancakeswap V2 styled pool
pancakeswap V3 styled pool
And it will withdraw liquidity from source pool and pipe it to correspond v4 cl-pool.

Features

No internal swap is involved
Support user add extra funds if desired
WETH source pool (ETH not support for v2 and v3) will be automatically piped into ETH pool in v4 and refund in the form of ETH

src/pool-cl/CLMigrator.sol

src/base/BaseMigrator.sol

src/pool-cl/CLMigrator.sol

ChefSnoopy · 2024-07-04T03:23:21Z

src/interfaces/IBaseMigrator.sol

+        uint256 amount0Min;
+        uint256 amount1Min;
+        // decide whether to collect fee
+        bool collectFee;


I suggest we can remove the collectFee flag.
Since we need to collect the liquidity amount , so it is better to collect the fee together.
Because fee number is always smaller than the liquidity amount.
Or we can keep this in the code, but FE use true as default.

I guess this works as a checkbox at frontend page. Will double confirm tho.

Per internal discussion: will keep it to give flexibility to user

ChefSnoopy · 2024-07-04T03:52:38Z

src/pool-cl/CLMigrator.sol

+
+    function migrateFromV2(
+        V2PoolParams calldata v2PoolParams,
+        INonfungiblePositionManager.MintParams calldata v4MintParams,


should we updated the v4MintParams.amount0Desired and amount1Desired based on the real withdraw amount from v2 or v3 ?
Because pool state is constantly changing, so maybe the withdraw amount is different from the number FE calculated.
v4MintParams.amount0Desired = amount0Received + extraAmount0;
v4MintParams.amount1Desired = amount1Received + extraAmount1;

Nice point. I can update to use the actual amount received. Also, do u think we should have a simple check here to see if the expected amount and actual amount way off

yes, we can check .
but if so , we need a rule to define the way off

Per internal discussion: The fields amount0Min and amount1Min in v3#decreaseLiquidity should be sufficient if price’s changed a log. We will add same checking rule for migrateFromV2 case

updated, same as the following one.. can take a look

ChefSnoopy · 2024-07-04T03:53:51Z

src/pool-cl/CLMigrator.sol

+            v4MintParams.poolKey.currency0, v4MintParams.poolKey.currency1, extraAmount0, extraAmount1
+        );
+
+        (,, uint256 amount0Consumed, uint256 amount1Consumed) = _addLiquidityToTargetPool(v4MintParams);


should we updated the v4MintParams.amount0Desired and amount1Desired based on the real withdraw amount from v2 or v3 ?
Same as the comment above.

ChefSnoopy · 2024-07-04T03:56:18Z

src/pool-cl/CLMigrator.sol

+            approveMax(params.poolKey.currency0, address(nonfungiblePositionManager));
+        }
+        approveMax(params.poolKey.currency1, address(nonfungiblePositionManager));
+


Good point.
I guess separate call is cheaper 🐶

Correct, i've initiated call separately

ChefMist · 2024-07-04T06:30:54Z

src/base/BaseMigrator.sol

+        }
+
+        // even if user sends native ETH, we still need to unwrap the part from source pool
+        if (address(token0) == NATIVE) {


(minor, non blocker) can consider using this currency0.isNative()

applies to other places where we check eg. address(token) == NATIVE

updated, thx for reminding

ChefMist · 2024-07-04T06:39:05Z

src/pool-cl/CLMigrator.sol

+        // TODO: compare the gas cost of multicall and two separate call
+        bytes[] memory data = new bytes[](2);
+        data[0] = abi.encodeWithSelector(nonfungiblePositionManager.mint.selector, params);
+        data[1] = abi.encodeWithSelector(nonfungiblePositionManager.refundETH.selector);


(optional) -- could it be more gas efficient if we only add refundEth for nativePair case?

eg.

if (nativePair) { bytes[] memory data = new bytes[](2); } else { bytes[] memory data = new bytes[](1); }

will do the gas comparison when start writing test

updated to avoid multi call which consumes more gas

ChefMist

no other feedback in general, structure looks clean and easy to understand for me

ChefSnoopy · 2024-07-08T07:00:19Z

src/pool-cl/CLMigrator.sol

+            if (amount1Received > amount1Consumed) {
+                v4MintParams.poolKey.currency1.transfer(v4MintParams.recipient, amount1Received - amount1Consumed);
+            }
+        }


Should we add event for migrateFromV2 and migrateFromV3 ?
Not sure whether FE need events to make statistics about migration.

given our v3 doesn't have the event and so far seems fine, i think we can leave it without events for now. (though open if we want to add)

ChefSnoopy · 2024-07-08T08:00:43Z

src/pool-cl/CLMigrator.sol

+        });
+        (,, uint256 amount0Consumed, uint256 amount1Consumed) = _addLiquidityToTargetPool(mintParams);
+
+        // refund if necessary, ETH is supported by CurrencyLib


should this use the (amount0Received + extraAmount0) > amount0Consumed ?

nice finding! fixed

ChefSnoopy · 2024-07-08T11:06:25Z

src/base/BaseMigrator.sol

+
+    function approveMax(Currency currency, address to) internal {
+        ERC20 token = ERC20(Currency.unwrap(currency));
+        if (token.allowance(address(this), to) == type(uint256).max) {


About this check condition , it will only apply for the new ERC20 token.
For old ERC20 tokens which deployed for a long time ago, the approve function did not have the "currentAllowance != type(uint256).max" checker , so it will always reduce the allowance , so for the old tokens will be approved every time when this token allowance was used before.
For example , you can check the Cake Token , no type(uint256).max when spent allowance.
https://bscscan.com/token/0x0E09FaBB73Bd3Ade0a17ECC321fD13a19e81cE82#code

so you need to change this condition , just like this :
token.allowance(address(this), to) > 0

thx for reminding, can take a look again

ChefSnoopy · 2024-07-09T03:13:54Z

src/base/BaseMigrator.sol

+    /// @dev receive extra tokens from user if necessary and normalize all the WETH to native ETH
+    function batchAndNormalizeTokens(Currency currency0, Currency currency1, uint256 extraAmount0, uint256 extraAmount1)
+        internal
+    {


I suggest we can check extraAmount0 and extraAmount1 at the beginning .
if(extraAmount0 == 0 && extraAmount1 == 0) {return;}

unwrapping WETH will still be needed because we might receive WETH from source pool. Or do u think i should move IWETH9(WETH9).withdraw(ERC20(WETH9).balanceOf(address(this))); to a separate function ?

good point.
you can skip this suggestion.

Or add extra logic about the WETH withdraw before return.
Separate function is also a good solution.

ChefSnoopy · 2024-07-09T03:27:21Z

src/pool-cl/CLMigrator.sol

+
+        (tokenId, liquidity, amount0Consumed, amount1Consumed) =
+            nonfungiblePositionManager.mint{value: nativePair ? params.amount0Desired : 0}(params);
+        if (nativePair) {


about this , i suggest we can add more condition.
if(nativePair && amount0Consumed < params.amount0Desired)

thx for reminding, updated

ChefSnoopy · 2024-07-09T06:00:56Z

src/base/BaseMigrator.sol

+        }
+
+        // even if user sends native ETH, we still need to unwrap the part from source pool
+        if (currency0.isNative()) {


should we check the ERC20(WETH9).balanceOf(address(this) > 0 ?
for example , if users try to migrate one v3 position which is out of range , so maybe it will get 0 amount for WETH , in this case , we still will go to call IWETH9(WETH9).withdraw , it will cost extra gas.

Agree, updated

ChefSnoopy · 2024-07-09T07:00:14Z

src/pool-cl/CLMigrator.sol

+        uint256 extraAmount0,
+        uint256 extraAmount1
+    ) external payable override {
+        IV3NonfungiblePositionManager.DecreaseLiquidityParams memory decreaseLiquidityParams =


ChefSnoopy · 2024-07-09T07:16:10Z

src/base/BaseMigrator.sol

@@ -45,6 +41,11 @@ contract BaseMigrator is IBaseMigrator, PeripheryImmutableState, Multicall, Self
        IV3NonfungiblePositionManager.DecreaseLiquidityParams memory decreaseLiquidityParams,
        bool collectFee
    ) internal returns (uint256 amount0Received, uint256 amount1Received) {
+        ///@dev make sure the caller is the owner of the token
+        /// otherwise once the token is approved to migrator, anyone can still money through this function


typo : still -> steal

…cross pool type

ChefMist · 2024-07-16T01:59:58Z

src/base/BaseMigrator.sol

+        }
+    }
+
+    /// @dev receive extra tokens from user if necessary and normalize all the WETH to native ETH


(not blocker)

could you add this somewhere? maybe in ICLMigrator for those core migrateFromV2 and migrateFromV3 method? feel like this might be good to flag out to external dev who are integrating

// @param: extraAmount0: if pool token0 is ETH and msg.value == 0, WETH will be taken from sender. Otherwise if pool token0 is ETH and msg.value !=0, method will assume user have sent extraAmount0 in msg.value```

will update this right before i merge all 3 PRs ( avoid annoying rebasing 😂

* feat: impl binPool migrator * refactor: restructure & renaming accordingly * test: add tests cases for binPool migrator * chore: renaming as well to align with clMigrator * fix: add check to prevent token mismatch between source and target pool * feat: added refundETH function and necessary comments * optimization: avoid duplicate external call when query v2/v3 pool info * feat: support selfPermitForERC721 (#62) * feat: support selfPermitForERC721 * docs: added comments suggesting users to use selfPermitERC721IfNecessary * test: added tests to prevent ppl from removing payable keyword from external functions

chefburger marked this pull request as draft July 2, 2024 10:22

chefburger force-pushed the feat/migrator branch 2 times, most recently from 9505919 to 326ebd8 Compare July 3, 2024 02:32

chefburger commented Jul 3, 2024

View reviewed changes

src/pool-cl/CLMigrator.sol Outdated Show resolved Hide resolved

chefburger force-pushed the feat/migrator branch from 326ebd8 to bd5b369 Compare July 3, 2024 03:00

ChefMist reviewed Jul 3, 2024

View reviewed changes

src/base/BaseMigrator.sol Outdated Show resolved Hide resolved

ChefMist reviewed Jul 3, 2024

View reviewed changes

src/pool-cl/CLMigrator.sol Outdated Show resolved Hide resolved

ChefMist reviewed Jul 3, 2024

View reviewed changes

src/pool-cl/CLMigrator.sol Outdated Show resolved Hide resolved

ChefSnoopy reviewed Jul 3, 2024

View reviewed changes

src/pool-cl/CLMigrator.sol Outdated Show resolved Hide resolved

chefburger force-pushed the feat/migrator branch 3 times, most recently from f5ac980 to b5cfd04 Compare July 3, 2024 12:23

ChefSnoopy reviewed Jul 4, 2024

View reviewed changes

ChefMist reviewed Jul 4, 2024

View reviewed changes

feat: implement v4Migrator for cl-pool

fbb0d1c

chefburger force-pushed the feat/migrator branch from b5cfd04 to fbb0d1c Compare July 4, 2024 07:45

chefburger added 3 commits July 4, 2024 15:51

feat: added same price slippage check for migrateFromV2

a1825df

feat: using actual receive amount when adding liquidity to v4

9c99091

test: added test for clMigrator

e2cd758

chefburger marked this pull request as ready for review July 8, 2024 06:29

Merge branch 'main' into feat/migrator

1553d15

chefburger changed the title ~~V4 Migrator~~ V4 CLMigrator Jul 8, 2024

ChefSnoopy reviewed Jul 8, 2024

View reviewed changes

fix: take into consideration of extra funds when calc refund

54d6049

ChefSnoopy reviewed Jul 8, 2024

View reviewed changes

optimization: avoid unnecessary approve in old token cases

a7773d4

chefburger force-pushed the feat/migrator branch from e9a7e56 to a7773d4 Compare July 8, 2024 14:05

ChefSnoopy reviewed Jul 9, 2024

View reviewed changes

optimization: avoid unnecessary refund call after adding liquidity

3dc30a4

chefburger force-pushed the feat/migrator branch from e28b5b3 to 3dc30a4 Compare July 9, 2024 04:29

ChefSnoopy reviewed Jul 9, 2024

View reviewed changes

optimization: avoid unnecessary WETH unwrap

663c018

ChefSnoopy reviewed Jul 9, 2024

View reviewed changes

fix: extra check so that non-owner can not steal funds from lpV3 token

ca5fd6f

ChefSnoopy reviewed Jul 9, 2024

View reviewed changes

chefburger mentioned this pull request Jul 9, 2024

V4 BinMigrator #59

Merged

chefburger added 3 commits July 9, 2024 15:29

typo

a548dfc

refactor: restructure baseMigrator a bit so that more can be reused a…

dd3a7ea

…cross pool type

chore: typo and renaming

8ed8199

ChefSnoopy previously approved these changes Jul 15, 2024

View reviewed changes

ChefMist reviewed Jul 16, 2024

View reviewed changes

ChefMist previously approved these changes Jul 16, 2024

View reviewed changes

chefburger dismissed stale reviews from ChefMist and ChefSnoopy via 3503133 July 18, 2024 07:17

docs: add explanation about the case where extra token0 is ETH

6cb8f80

chefburger merged commit bcbacc9 into main Jul 18, 2024
1 check passed

chefburger deleted the feat/migrator branch July 18, 2024 07:23

V4 CLMigrator #51

V4 CLMigrator #51

Conversation

chefburger commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChefMist Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChefMist Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChefMist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChefMist Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chefburger commented Jul 2, 2024 •

edited

Loading

ChefMist Jul 4, 2024 •

edited

Loading

ChefMist Jul 4, 2024 •

edited

Loading

ChefMist Jul 16, 2024 •

edited

Loading