Before after add/remove liquidity, remove all modifyPosition hooks #434

rileydcampbell · 2023-12-06T21:03:46Z

Which issue does this pull request resolve?

This PR fixes #350. The main idea is:

Right now, beforeModifyPosition is a risky permission. Liquidity providers need to audit and trust the hook code or risk never being able to withdraw their money. That makes it harder to fearlessly integrate pools that have this permission.

Description of changes

We broke up the modify position hook into an addLiquidity and removeLiquidity hook. This allows for a better separation of permissions. The following changes were made to implement these hooks.

Add before/after addLiquidity
Add before/after removeLiquidity
Remove modifyPosition hooks
Update existing tests and added new ones

…-after-mint

…into before-after-mint

…-after-add/removeLiquidity

rileydcampbell · 2023-12-07T22:33:43Z

test/AccessLock.t.sol

+        // TODO: Maybe due to a rounding issue, but balanceOfBefore0 == balanceOfAfter0 ?
+        assertTrue(balanceOfBefore0 - balanceOfAfter0 <= 1);
+        assertTrue(balanceOfBefore1 - balanceOfAfter1 - amount <= 1);


Not sure why I had an off by one issue here? Is it possible that it is a rounding issue?

Ya should equal, that's werid

…-after-add/removeLiquidity

hensha256

I feel like theres improvements in efficiency of the internal code that can be made now that we have separation of add/remove liquidity functions.

For example, in the Pool library in modifyPosition, it has to do checks

if (params.liquidityDelta > 0) {
   /// aka if this is an ADD liquidity then do X
} 
if (params.liquidityDelta < 0) {
   /// aka if this is a REMOVE liquidity then do Y
}

this PR gives the nice separation of entry points, but then still leads to the same internal flow of checking whether liquidityDelta is positive or negative, even though that is actually known in advance

I think we should maybe investigate splitting pool logic into internal addLiquidity and removeLiquidity, where removeLiquidity never has to do any of the params.liquidityDelta > 0 checks and vice versa?

rileydcampbell · 2023-12-08T19:58:13Z

@hensha256 Definitely agree! I can take a closer look at the Pool logic to see how that might simplify things. This would be as a follow up yeah? Not blocking

snreynolds

seems like a good structure although hard to give a full review knowing a lot of this will change with Alice's and Mark's changes... could you pull those in? 😄

src/PoolManager.sol

src/interfaces/IPoolManager.sol

src/PoolManager.sol

rileydcampbell · 2023-12-08T20:18:27Z

seems like a good structure although hard to give a full review knowing a lot of this will change with Alice's and Mark's changes... could you pull those in? 😄

Yeah will do! Might take me a sec to do the refactor/fix all the merge conflicts

hensha256 · 2023-12-08T21:16:45Z

This would be as a follow up yeah? Not blocking

Yeah we can make it a followup, please can you open a github issue so we dont forget?

src/PoolManager.sol

…-after-add/removeLiquidity

rileydcampbell · 2023-12-12T18:04:35Z

src/libraries/Hooks.sol

    ) internal returns (bool shouldExecute) {
-        if (self.hasPermission(BEFORE_MODIFY_POSITION_FLAG)) {
-            shouldExecute = self.callHookNoopable(
-                abi.encodeWithSelector(IHooks.beforeModifyPosition.selector, msg.sender, key, params, hookData)
-            );
+        if (params.liquidityDelta >= 0) {
+            if (key.hooks.hasPermission(BEFORE_ADD_LIQUIDITY_FLAG)) {
+                shouldExecute = self.callHookNoopable(
+                    abi.encodeWithSelector(IHooks.beforeAddLiquidity.selector, msg.sender, key, params, hookData)
+                );
+            } else {
+                shouldExecute = true;
+            }
        } else {
-            return true;
+            if (key.hooks.hasPermission(BEFORE_REMOVE_LIQUIDITY_FLAG)) {
+                shouldExecute = self.callHookNoopable(
+                    abi.encodeWithSelector(IHooks.beforeRemoveLiquidity.selector, msg.sender, key, params, hookData)
+                );
+            } else {
+                shouldExecute = true;
+            }


Feel like this looks a little ugly, could also change to something like:

/// @notice calls beforeModifyPosition hook if permissioned and validates return value function beforeModifyPosition( IHooks self, PoolKey memory key, IPoolManager.ModifyPositionParams memory params, bytes calldata hookData ) internal returns (bool shouldExecute) { if (params.liquidityDelta >= 0 && key.hooks.hasPermission(BEFORE_ADD_LIQUIDITY_FLAG)) { shouldExecute = self.callHookNoopable( abi.encodeWithSelector(IHooks.beforeAddLiquidity.selector, msg.sender, key, params, hookData) ); } else if (params.liquidityDelta < 0 && key.hooks.hasPermission(BEFORE_REMOVE_LIQUIDITY_FLAG)) { shouldExecute = self.callHookNoopable( abi.encodeWithSelector(IHooks.beforeRemoveLiquidity.selector, msg.sender, key, params, hookData) ); } else { shouldExecute = true; } }

It's a little more gas ~100 but maybe that's ok?

I think we should expose separate getters for querying beforeAdd/RemoveLiquidity and deprecating beforeModifiyPosition + afterModifyPosition so you don't need this conditional

I think I prefer this branching. And @zhongeric I think Mark and I talked about this and we kinda liked having the beforeModifyPos/afterModifyPos as the entrypoints in the Hooks library so that we are removing the liquidityDelta logic from core codepaths

zhongeric · 2023-12-12T18:11:22Z

src/libraries/Hooks.sol

    ) internal returns (bool shouldExecute) {
-        if (self.hasPermission(BEFORE_MODIFY_POSITION_FLAG)) {
-            shouldExecute = self.callHookNoopable(
-                abi.encodeWithSelector(IHooks.beforeModifyPosition.selector, msg.sender, key, params, hookData)
-            );
+        if (params.liquidityDelta >= 0) {
+            if (key.hooks.hasPermission(BEFORE_ADD_LIQUIDITY_FLAG)) {
+                shouldExecute = self.callHookNoopable(
+                    abi.encodeWithSelector(IHooks.beforeAddLiquidity.selector, msg.sender, key, params, hookData)
+                );
+            } else {
+                shouldExecute = true;
+            }
        } else {
-            return true;
+            if (key.hooks.hasPermission(BEFORE_REMOVE_LIQUIDITY_FLAG)) {
+                shouldExecute = self.callHookNoopable(
+                    abi.encodeWithSelector(IHooks.beforeRemoveLiquidity.selector, msg.sender, key, params, hookData)
+                );
+            } else {
+                shouldExecute = true;
+            }


I think we should expose separate getters for querying beforeAdd/RemoveLiquidity and deprecating beforeModifiyPosition + afterModifyPosition so you don't need this conditional

src/test/NoOpTestHooks.sol

zhongeric · 2023-12-12T18:17:45Z

test/AccessLock.t.sol

+        // TODO: Maybe due to a rounding issue, but balanceOfBefore0 == balanceOfAfter0 ?
+        assertTrue(balanceOfBefore0 - balanceOfAfter0 <= 1);
+        assertTrue(balanceOfBefore1 - balanceOfAfter1 - amount <= 1);


Ya should equal, that's werid

test/AccessLock.t.sol

snreynolds

One last overall

Should we be hitting the beforeAddLiquidity logic for liquidityParams of 0? I think in the worst case someone could revert on just collecting fees but not wanting to change their position. Either we don't call out to hooks or maybe have the 0 liquidityParams branch out to the afterAddLiquidity branch?

Also should we change name to modifyLiquidity()?

rileydcampbell · 2023-12-13T21:40:57Z

I removed any of the add/remove liquidity hook calls for a liquidityDelta == 0.

snreynolds

just last name change suggestion but looks rlly good!

snreynolds · 2023-12-14T16:09:14Z

src/interfaces/IPoolManager.sol

+    /// @param params The parameters for modifying the liquidity
+    /// @param hookData Any data to pass to the callback, via `ILockCallback(msg.sender).lockAcquired(data)`
+    /// @return delta The balance delta of the liquidity
+    function modifyLiquidity(PoolKey memory key, ModifyPositionParams memory params, bytes calldata hookData)


laaaast lil nit: ModifyLiquidityParams ?

snreynolds · 2023-12-14T16:28:21Z

src/libraries/Hooks.sol

@@ -161,12 +168,16 @@ library Hooks {
        IPoolManager.ModifyPositionParams memory params,
        bytes calldata hookData
    ) internal returns (bool shouldExecute) {


and same name change on these functions? Just thinking so that they are all standardized

beforeModifyPosition -> beforeModifyLiquidity
afterModifyPosition -> afterModifyLiquidity

snreynolds

🚢

…niswap#434) * Add before/after add/remove liquidity hooks --------- Co-authored-by: Eric Zhong <[email protected]>

zhongeric and others added 16 commits November 13, 2023 16:27

Rip out beforeModifyPosition and after, add mints

8bf9f1b

forge fmt

5babed0

Add explicit before/after mint hook calls

bb11369

Add mintPosition

de292c1

fix wrong test assumption

c6a223c

forge fmt

38a1e5d

natspec

c7e4a72

Merge branch 'main' into before-after-mint

b24c159

chore: change mintPosition -> addLiquidity

90af549

merged changes

97779f5

fix tests

b0fea97

Merge branch 'main' of https://github.com/Uniswap/v4-core into before…

25314e3

…-after-mint

Merge branch 'main' of https://github.com/Uniswap/v4-core into before…

3f4467d

…-after-mint

More tests

1a75990

More tests

33e602d

Merge branch 'before-after-mint' of https://github.com/Uniswap/v4-core …

21514b4

…into before-after-mint

snreynolds mentioned this pull request Dec 6, 2023

Add {before/after}mint hooks, remove all modifyPosition hooks #402

Closed

rileydcampbell added 3 commits December 6, 2023 18:21

Change modify position params for PoolManager

518737a

Merge branch 'main' of https://github.com/Uniswap/v4-core into before…

3fd4079

…-after-add/removeLiquidity

Fixed PoolModifyPositionTest and some more breaking tests.

cee4d82

rileydcampbell force-pushed the before-after-add/removeLiquidity branch from 7ec6d8d to cee4d82 Compare December 7, 2023 22:31

rileydcampbell commented Dec 7, 2023

View reviewed changes

New gas snapshots

0d37920

rileydcampbell marked this pull request as ready for review December 7, 2023 22:39

rileydcampbell added 3 commits December 7, 2023 17:10

fix test comments

17b439a

Merge branch 'main' of https://github.com/Uniswap/v4-core into before…

689fab4

…-after-add/removeLiquidity

fix: use bound instead of vm.assume

399f250

hensha256 reviewed Dec 8, 2023

View reviewed changes

snreynolds reviewed Dec 8, 2023

View reviewed changes

src/PoolManager.sol Outdated Show resolved Hide resolved

src/interfaces/IPoolManager.sol Outdated Show resolved Hide resolved

snreynolds reviewed Dec 8, 2023

View reviewed changes

src/PoolManager.sol Outdated Show resolved Hide resolved

marktoda reviewed Dec 8, 2023

View reviewed changes

src/PoolManager.sol Outdated Show resolved Hide resolved

rileydcampbell added 2 commits December 11, 2023 20:46

pull in changes and refactor hook calls

6c497bc

Merge branch 'main' of https://github.com/Uniswap/v4-core into before…

832e6b1

…-after-add/removeLiquidity

rileydcampbell force-pushed the before-after-add/removeLiquidity branch from b9f2398 to 832e6b1 Compare December 12, 2023 02:50

fix tests

435c914

rileydcampbell force-pushed the before-after-add/removeLiquidity branch from 3912e4a to 435c914 Compare December 12, 2023 03:18

rileydcampbell requested review from snreynolds, hensha256 and marktoda December 12, 2023 03:20

Georgezhang714 approved these changes Dec 12, 2023

View reviewed changes

rileydcampbell commented Dec 12, 2023

View reviewed changes

zhongeric reviewed Dec 12, 2023

View reviewed changes

addressed feedback

389789c

snreynolds reviewed Dec 12, 2023

View reviewed changes

Change entry point name and no hooks on liquidity delta zero

8c5e457

rileydcampbell force-pushed the before-after-add/removeLiquidity branch from 0db7782 to 8c5e457 Compare December 13, 2023 21:42

rileydcampbell requested a review from snreynolds December 13, 2023 21:43

call before/afterRemove Liquidity hooks on zero liquidity delta

1260a27

rileydcampbell requested a review from zhongeric December 14, 2023 00:58

snreynolds reviewed Dec 14, 2023

View reviewed changes

rileydcampbell added 2 commits December 14, 2023 13:06

fix naming

43d5aea

change event name

e408681

snreynolds approved these changes Dec 14, 2023

View reviewed changes

rileydcampbell merged commit 36160ce into main Dec 14, 2023
4 checks passed

rileydcampbell deleted the before-after-add/removeLiquidity branch December 14, 2023 19:41

hyunchel pushed a commit to hyunchel/v4-core that referenced this pull request Feb 21, 2024

Before after add/remove liquidity, remove all modifyPosition hooks (U…

f07fcd3

…niswap#434) * Add before/after add/remove liquidity hooks --------- Co-authored-by: Eric Zhong <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Before after add/remove liquidity, remove all modifyPosition hooks #434

Before after add/remove liquidity, remove all modifyPosition hooks #434

rileydcampbell commented Dec 6, 2023 •

edited

Loading

rileydcampbell Dec 7, 2023 •

edited

Loading

zhongeric Dec 12, 2023

hensha256 left a comment

rileydcampbell commented Dec 8, 2023

snreynolds left a comment

rileydcampbell commented Dec 8, 2023

hensha256 commented Dec 8, 2023

rileydcampbell Dec 12, 2023

zhongeric Dec 12, 2023

snreynolds Dec 12, 2023

zhongeric Dec 12, 2023

zhongeric Dec 12, 2023

snreynolds left a comment

rileydcampbell commented Dec 13, 2023

snreynolds left a comment

snreynolds Dec 14, 2023

snreynolds Dec 14, 2023

rileydcampbell Dec 14, 2023

snreynolds left a comment

Before after add/remove liquidity, remove all modifyPosition hooks #434

Before after add/remove liquidity, remove all modifyPosition hooks #434

Conversation

rileydcampbell commented Dec 6, 2023 • edited Loading

Which issue does this pull request resolve?

Description of changes

rileydcampbell Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hensha256 left a comment

Choose a reason for hiding this comment

rileydcampbell commented Dec 8, 2023

snreynolds left a comment

Choose a reason for hiding this comment

rileydcampbell commented Dec 8, 2023

hensha256 commented Dec 8, 2023

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

snreynolds left a comment

Choose a reason for hiding this comment

rileydcampbell commented Dec 13, 2023

snreynolds 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

snreynolds left a comment

Choose a reason for hiding this comment

rileydcampbell commented Dec 6, 2023 •

edited

Loading

rileydcampbell Dec 7, 2023 •

edited

Loading