[PART-1] enable hooks to return deltas to influence pool behavior #44

chefburger · 2024-05-15T08:51:03Z

This PR allows hooks contract to return extra parameter which can have impact on pool's behavior. More specifically:

beforeSwap: a extra value is returned which will be added onto amtSpecified/amtIn which can be used to update current swap amount
afterSwap: a new value is returned which will be combined with the value returned by beforeSwap. Those two together work as a update to the balanceDelta i.e. through it payment can be transferred between user and hook contract.
afterAddLiquidity/afterMint/afterRemoveLiquidity/afterBurn: a balanceDelta is returned which will be added onto user's balanceDelta i.e. through it payment can be transferred between user and hook contract.

NO_OP feature has been removed because its functionality has overlapped with this feature:

for swap, consider value returned = -amtSpecified/amtIn
for modifyLiquidity: consider doing operation in opposite direction in afterXXX as user

However, there are also some reasons that NO_OP might be the way to go:

NO_OP can naturally consume less gas
When mint position in binPool and compositionFee is generated, in this case operation in opposite direction won't help
donate

chefburger · 2024-05-15T08:56:33Z

src/pool-bin/BinPoolManager.sol

@@ -135,27 +136,19 @@ contract BinPoolManager is IBinPoolManager, ProtocolFees, Extsload {
        whenNotPaused
        returns (BalanceDelta delta)
    {
+        if (amountIn == 0) revert InsufficientAmountIn();


This check has been moved from BinPool.sol to here otherwise the call will revert if hook contract updates the actual amountIn to 0 through beforeSwap

chefburger · 2024-05-15T09:02:00Z

src/pool-bin/libraries/BinHooks.sol

+        bytes calldata hookData
+    ) internal returns (BalanceDelta callerDelta, BalanceDelta hookDelta) {
+        IBinHooks hooks = IBinHooks(address(key.hooks));
+        callerDelta = delta;


if skip i.e. caller is already hook, then callerDelta is unchanged and hookDelta is 0

chefburger · 2024-05-15T09:03:32Z

src/pool-bin/libraries/BinHooks.sol

+
+    function beforeSwap(PoolKey memory key, bool swapForY, uint128 amountIn, bytes calldata hookData)
+        internal
+        returns (uint128 amountToSwap, int128 hookDeltaSpecified)


This is uint128 because unlike cl-pool, in bin-pool users are always specifying exactlyIn amount

chefburger · 2024-05-15T09:07:21Z

src/pool-bin/libraries/BinHooks.sol

+            }
+        }
+
+        if (hookDeltaUnspecified != 0 || hookDeltaSpecified != 0) {


This can still be true even "afterSwapReturnDelta" is disabled

is it better to rename hookDeltaSpecified as beforeSwapHookDeltaSpecified?

already updated in part3

chefburger · 2024-05-15T09:09:07Z

src/pool-bin/libraries/BinHooks.sol

+                revert Hooks.InvalidHookResponse();
+            }
+
+            if (!key.parameters.hasOffsetEnabled(HOOKS_AFTER_SWAP_RETURNS_DELTA_OFFSET)) {


Potentially optimization: skip decoding the second return value when afterSwapReturnDelta not set

add a //todo comment in the code if possible for these cases

added in part3

chefburger · 2024-05-18T06:27:55Z

src/pool-cl/libraries/CLPool.sol

@@ -145,7 +142,7 @@ library CLPool {
        }

        // Fees earned from LPing are removed from the pool balance.
-        feeDelta = toBalanceDelta(feesOwed0.toInt128(), feesOwed1.toInt128());
+        feeDelta = toBalanceDelta(-feesOwed0.toInt128(), -feesOwed1.toInt128());


feeDelta must be negative i.e. both feeDelta.amount0 <= 0 and feeDelta.amount1 <= 0 because that's the fee owed to the user by the pool. Our rule for BalanceDelta is:

Positive number means user owes the pool

Negative number means pool owes user

TO CONFIRM: If we want to flip to sign for all the delta across the repo

ChefMist · 2024-05-20T07:37:40Z

NO_OP feature has been removed because its functionality has overlapped with this feature:

1. for swap, consider value returned = -amtSpecified/amtIn
2. for modifyLiquidity: consider doing operation in opposite direction in afterXXX as user

Do you mean that if we want to replicate no-op for swap/modifyLiquidity, the hook should ensure that balanceDelta accounted for both hook/caller is 0?

chefburger · 2024-05-22T10:37:19Z

NO_OP feature has been removed because its functionality has overlapped with this feature:

1. for swap, consider value returned = -amtSpecified/amtIn
2. for modifyLiquidity: consider doing operation in opposite direction in afterXXX as user
Do you mean that if we want to replicate no-op for swap/modifyLiquidity, the hook should ensure that balanceDelta accounted for both hook/caller is 0?

yes, i guess so, see:

// CLPoolManager.sol
function swap() {'

  // ...
  if (hookDelta != BalanceDeltaLibrary.ZERO_DELTA) {
      vault.accountPoolBalanceDelta(key, hookDelta, address(key.hooks));
  }

  /// @dev delta already includes protocol fee
  /// all tokens go into the vault
  vault.accountPoolBalanceDelta(key, delta, msg.sender);
}

Another way could be using settleFor if caller/router would like to pay for the hook

ChefMist · 2024-05-24T02:58:18Z

src/pool-bin/BinPoolManager.sol

-                // Sentinel return value used to signify that a NoOp occurred.
-                return (BalanceDeltaLibrary.MAXIMUM_DELTA, binId);
-            } else if (selector != IBinHooks.beforeDonate.selector) {
+            if (hooks.beforeDonate(msg.sender, key, amount0, amount1, hookData) != IBinHooks.beforeDonate.selector) {


im thinking if we can create an issue to refactor (not in this PR but after all parts are merged) -- eg. move all hook call to CLHooks/BinHooks to make it easier to read for the devs. what do you think?

// some are without CLHooks ICLHooks hooks = ICLHooks(address(key.hooks)); if (key.parameters.shouldCall(HOOKS_BEFORE_DONATE_OFFSET, hooks)) { if (hooks.beforeDonate(msg.sender, key, amount0, amount1, hookData) != ICLHooks.beforeDonate.selector) { revert Hooks.InvalidHookResponse(); } } // some are using CLHooks (delta, hookDelta) = CLHooks.afterModifyLiquidity(key, params, delta + feeDelta, hookData);

Agree with that, it brings better readability

ChefMist · 2024-05-24T06:57:17Z

src/pool-bin/libraries/BinHooks.sol

+        }
+
+        if (hookDeltaUnspecified != 0 || hookDeltaSpecified != 0) {
+            hookDelta = swapForY


curious why here is only checking for swapForY while CLPool is checking amountSpecified as well (params.amountSpecified > 0 == params.zeroForOne)

ah got it. params.amountSpecified > 0 would tell us if its exactInput or exactOutput but for bin case, its always exactIn

exactly 😄

* feat: add salt to distinguish positions for same owner in cl-pool * feat: add salt to distinguish positions for same owner in bin-pool * optimiazaiton: small tweaks per comments * test: added salt != 0 cases * [PART-3] allow `beforeSwap` to return temp lpFee & delta for both sides (#47) * feat: finish updating for cl-pool * feat: finish updating for bin-pool * chore: correct comments removed unnecessary isValid function

ChefMist

re-approved since we merged part 3 and part 2 in this PR

chefburger commented May 15, 2024

View reviewed changes

chefburger marked this pull request as draft May 15, 2024 09:14

feat: enable hooks to return deltas to influence pool behavior

395eb86

chefburger force-pushed the feat/hook-return-deltas branch from ea853f2 to 395eb86 Compare May 15, 2024 09:37

chefburger added 2 commits May 18, 2024 14:17

test: fix & add test cases hook returns delta

a64d38f

Merge branch 'main' into feat/hook-return-deltas

f4030b5

chefburger marked this pull request as ready for review May 18, 2024 06:24

chefburger commented May 18, 2024

View reviewed changes

ChefMist self-requested a review May 20, 2024 03:09

chefburger changed the title ~~feat: enable hooks to return deltas to influence pool behavior~~ [PART-1] enable hooks to return deltas to influence pool behavior May 21, 2024

ChefMist reviewed May 24, 2024

View reviewed changes

ChefMist previously approved these changes May 24, 2024

View reviewed changes

ChefMist reviewed May 24, 2024

View reviewed changes

ChefMist mentioned this pull request May 27, 2024

[Refactor] move all hook call to CLHooks/BinHooks to make it easier to read for the devs #48

Closed

chefburger dismissed ChefMist’s stale review via 366e112 May 27, 2024 05:53

ChefMist approved these changes May 27, 2024

View reviewed changes

ChefSnoopy approved these changes May 27, 2024

View reviewed changes

chefburger merged commit 47315dc into main May 27, 2024
2 checks passed

chefburger deleted the feat/hook-return-deltas branch May 27, 2024 06:16

ChefMist mentioned this pull request Jun 6, 2024

chore: Update with latest v4-core changes pancakeswap/pancake-v4-periphery-deprecated#32

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PART-1] enable hooks to return deltas to influence pool behavior #44

[PART-1] enable hooks to return deltas to influence pool behavior #44

chefburger commented May 15, 2024 •

edited

Loading

chefburger May 15, 2024 •

edited

Loading

chefburger May 15, 2024

chefburger May 15, 2024

chefburger May 15, 2024

ChefMist May 20, 2024

chefburger May 22, 2024

chefburger May 15, 2024 •

edited

Loading

ChefMist May 20, 2024

chefburger May 22, 2024

chefburger May 18, 2024 •

edited

Loading

ChefMist commented May 20, 2024

chefburger commented May 22, 2024

ChefMist May 24, 2024 •

edited

Loading

chefburger May 24, 2024

ChefMist May 24, 2024 •

edited

Loading

chefburger May 24, 2024

ChefMist left a comment

[PART-1] enable hooks to return deltas to influence pool behavior #44

[PART-1] enable hooks to return deltas to influence pool behavior #44

Conversation

chefburger commented May 15, 2024 • edited Loading

chefburger May 15, 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

chefburger May 15, 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 May 18, 2024 • edited Loading

Choose a reason for hiding this comment

ChefMist commented May 20, 2024

chefburger commented May 22, 2024

ChefMist May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChefMist May 24, 2024 • edited Loading

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

chefburger commented May 15, 2024 •

edited

Loading

chefburger May 15, 2024 •

edited

Loading

chefburger May 15, 2024 •

edited

Loading

chefburger May 18, 2024 •

edited

Loading

ChefMist May 24, 2024 •

edited

Loading

ChefMist May 24, 2024 •

edited

Loading