Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

skip calls to hooks when msg.sender is hook contract + test #503

Merged
merged 19 commits into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity with empty hook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
311243
311305
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity with native token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
191422
191404
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
191403
191385
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 1 token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
131137
131119
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 2 tokens.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
175960
175942
2 changes: 1 addition & 1 deletion .forge-snapshots/poolManager bytecode size.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
22875
22863
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity with empty hook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
98020
98002
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity with native token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
199393
199375
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
195670
195652
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap with native.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
186039
185948
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
194579
194488
Original file line number Diff line number Diff line change
@@ -1 +1 @@
116698
116607
2 changes: 1 addition & 1 deletion .forge-snapshots/swap against liquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
104165
104074
2 changes: 1 addition & 1 deletion .forge-snapshots/swap burn 6909 for input.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
124314
124223
2 changes: 1 addition & 1 deletion .forge-snapshots/swap burn native 6909 for input.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
120267
120176
2 changes: 1 addition & 1 deletion .forge-snapshots/swap mint native output as 6909.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
188597
188506
2 changes: 1 addition & 1 deletion .forge-snapshots/swap mint output as 6909.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
205406
205315
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with dynamic fee.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
137470
137379
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
104143
104052
2 changes: 1 addition & 1 deletion .forge-snapshots/update dynamic fee in before swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
188472
188421
25 changes: 13 additions & 12 deletions src/libraries/Hooks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,13 @@ library Hooks {
}

/// @notice Ensures that the hook address includes at least one hook flag or dynamic fees, or is the 0 address
/// @param hook The hook to verify
function isValidHookAddress(IHooks hook, uint24 fee) internal pure returns (bool) {
/// @param self The hook to verify
function isValidHookAddress(IHooks self, uint24 fee) internal pure returns (bool) {
// If there is no hook contract set, then fee cannot be dynamic
// If a hook contract is set, it must have at least 1 flag set, or have a dynamic fee
return address(hook) == address(0)
return address(self) == address(0)
? !fee.isDynamicFee()
: (uint160(address(hook)) >= AFTER_DONATE_FLAG || fee.isDynamicFee());
: (uint160(address(self)) >= AFTER_DONATE_FLAG || fee.isDynamicFee());
}

/// @notice performs a hook call using the given calldata on the given hook
Expand All @@ -96,6 +96,7 @@ library Hooks {

/// @notice performs a hook call using the given calldata on the given hook
function callHook(IHooks self, bytes memory data) internal {
if (msg.sender == address(self)) return;
(bytes4 expectedSelector, bytes4 selector) = _callHook(self, data);

if (selector != expectedSelector) {
Expand Down Expand Up @@ -132,9 +133,9 @@ library Hooks {
IPoolManager.ModifyLiquidityParams memory params,
bytes calldata hookData
) internal {
if (params.liquidityDelta > 0 && key.hooks.hasPermission(BEFORE_ADD_LIQUIDITY_FLAG)) {
if (params.liquidityDelta > 0 && self.hasPermission(BEFORE_ADD_LIQUIDITY_FLAG)) {
self.callHook(abi.encodeWithSelector(IHooks.beforeAddLiquidity.selector, msg.sender, key, params, hookData));
} else if (params.liquidityDelta <= 0 && key.hooks.hasPermission(BEFORE_REMOVE_LIQUIDITY_FLAG)) {
} else if (params.liquidityDelta <= 0 && self.hasPermission(BEFORE_REMOVE_LIQUIDITY_FLAG)) {
self.callHook(
abi.encodeWithSelector(IHooks.beforeRemoveLiquidity.selector, msg.sender, key, params, hookData)
);
Expand All @@ -149,11 +150,11 @@ library Hooks {
BalanceDelta delta,
bytes calldata hookData
) internal {
if (params.liquidityDelta > 0 && key.hooks.hasPermission(AFTER_ADD_LIQUIDITY_FLAG)) {
if (params.liquidityDelta > 0 && self.hasPermission(AFTER_ADD_LIQUIDITY_FLAG)) {
self.callHook(
abi.encodeWithSelector(IHooks.afterAddLiquidity.selector, msg.sender, key, params, delta, hookData)
);
} else if (params.liquidityDelta <= 0 && key.hooks.hasPermission(AFTER_REMOVE_LIQUIDITY_FLAG)) {
} else if (params.liquidityDelta <= 0 && self.hasPermission(AFTER_REMOVE_LIQUIDITY_FLAG)) {
self.callHook(
abi.encodeWithSelector(IHooks.afterRemoveLiquidity.selector, msg.sender, key, params, delta, hookData)
);
Expand All @@ -164,7 +165,7 @@ library Hooks {
function beforeSwap(IHooks self, PoolKey memory key, IPoolManager.SwapParams memory params, bytes calldata hookData)
internal
{
if (key.hooks.hasPermission(BEFORE_SWAP_FLAG)) {
if (self.hasPermission(BEFORE_SWAP_FLAG)) {
self.callHook(abi.encodeWithSelector(IHooks.beforeSwap.selector, msg.sender, key, params, hookData));
}
}
Expand All @@ -177,7 +178,7 @@ library Hooks {
BalanceDelta delta,
bytes calldata hookData
) internal {
if (key.hooks.hasPermission(AFTER_SWAP_FLAG)) {
if (self.hasPermission(AFTER_SWAP_FLAG)) {
self.callHook(abi.encodeWithSelector(IHooks.afterSwap.selector, msg.sender, key, params, delta, hookData));
}
}
Expand All @@ -186,7 +187,7 @@ library Hooks {
function beforeDonate(IHooks self, PoolKey memory key, uint256 amount0, uint256 amount1, bytes calldata hookData)
internal
{
if (key.hooks.hasPermission(BEFORE_DONATE_FLAG)) {
if (self.hasPermission(BEFORE_DONATE_FLAG)) {
self.callHook(
abi.encodeWithSelector(IHooks.beforeDonate.selector, msg.sender, key, amount0, amount1, hookData)
);
Expand All @@ -197,7 +198,7 @@ library Hooks {
function afterDonate(IHooks self, PoolKey memory key, uint256 amount0, uint256 amount1, bytes calldata hookData)
internal
{
if (key.hooks.hasPermission(AFTER_DONATE_FLAG)) {
if (self.hasPermission(AFTER_DONATE_FLAG)) {
self.callHook(
abi.encodeWithSelector(IHooks.afterDonate.selector, msg.sender, key, amount0, amount1, hookData)
);
Expand Down
13 changes: 4 additions & 9 deletions src/test/PoolSwapTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,16 @@ contract PoolSwapTest is PoolTestBase {

CallbackData memory data = abi.decode(rawData, (CallbackData));

(,, uint256 reserveBefore0, int256 deltaBefore0) =
_fetchBalances(data.key.currency0, data.sender, address(this));
(,, uint256 reserveBefore1, int256 deltaBefore1) =
_fetchBalances(data.key.currency1, data.sender, address(this));
(,,, int256 deltaBefore0) = _fetchBalances(data.key.currency0, data.sender, address(this));
(,,, int256 deltaBefore1) = _fetchBalances(data.key.currency1, data.sender, address(this));

require(deltaBefore0 == 0, "deltaBefore0 is not equal to 0");
require(deltaBefore1 == 0, "deltaBefore1 is not equal to 0");

BalanceDelta delta = manager.swap(data.key, data.params, data.hookData);

(,, uint256 reserveAfter0, int256 deltaAfter0) = _fetchBalances(data.key.currency0, data.sender, address(this));
(,, uint256 reserveAfter1, int256 deltaAfter1) = _fetchBalances(data.key.currency1, data.sender, address(this));

require(reserveBefore0 == reserveAfter0, "reserveBefore0 is not equal to reserveAfter0");
require(reserveBefore1 == reserveAfter1, "reserveBefore1 is not equal to reserveAfter1");
(,,, int256 deltaAfter0) = _fetchBalances(data.key.currency0, data.sender, address(this));
(,,, int256 deltaAfter1) = _fetchBalances(data.key.currency1, data.sender, address(this));

if (data.params.zeroForOne) {
if (data.params.amountSpecified < 0) {
Expand Down
49 changes: 49 additions & 0 deletions src/test/SkipCallsTestHook.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.24;

import {Hooks} from "../libraries/Hooks.sol";
import {BaseTestHooks} from "./BaseTestHooks.sol";
import {IHooks} from "../interfaces/IHooks.sol";
import {IPoolManager} from "../interfaces/IPoolManager.sol";
import {PoolKey} from "../types/PoolKey.sol";
import {BalanceDelta} from "../types/BalanceDelta.sol";
import {PoolId, PoolIdLibrary} from "../types/PoolId.sol";
import {IERC20Minimal} from "../interfaces/external/IERC20Minimal.sol";
import {CurrencyLibrary, Currency} from "../types/Currency.sol";
import {PoolTestBase} from "./PoolTestBase.sol";
import {Test} from "forge-std/Test.sol";

contract SkipCallsTestHook is BaseTestHooks, Test {
using PoolIdLibrary for PoolKey;
using Hooks for IHooks;

uint256 public counter;
IPoolManager manager;
uint24 internal fee;

function setManager(IPoolManager _manager) external {
manager = _manager;
}

function beforeSwap(address, PoolKey calldata key, IPoolManager.SwapParams calldata params, bytes calldata hookData)
external
override
returns (bytes4)
{
counter++;
callSwap(key, params, hookData);
return IHooks.beforeSwap.selector;
}

function callSwap(PoolKey calldata key, IPoolManager.SwapParams calldata params, bytes calldata hookData) public {
IPoolManager(manager).swap(key, params, hookData);
address payer = abi.decode(hookData, (address));
int256 delta0 = IPoolManager(manager).currencyDelta(address(this), key.currency0);
assertEq(delta0, params.amountSpecified);
int256 delta1 = IPoolManager(manager).currencyDelta(address(this), key.currency1);
assert(delta1 > 0);
IERC20Minimal(Currency.unwrap(key.currency0)).transferFrom(payer, address(manager), uint256(-delta0));
manager.settle(key.currency0);
manager.take(key.currency1, payer, uint256(delta1));
}
}
52 changes: 52 additions & 0 deletions test/SkipCallsTestHook.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.20;

import {Test} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";
import {PoolId, PoolIdLibrary} from "../src/types/PoolId.sol";
import {Hooks} from "../src/libraries/Hooks.sol";
import {SwapFeeLibrary} from "../src/libraries/SwapFeeLibrary.sol";
import {IPoolManager} from "../src/interfaces/IPoolManager.sol";
import {IProtocolFees} from "../src/interfaces/IProtocolFees.sol";
import {IHooks} from "../src/interfaces/IHooks.sol";
import {PoolKey} from "../src/types/PoolKey.sol";
import {PoolManager} from "../src/PoolManager.sol";
import {PoolSwapTest} from "../src/test/PoolSwapTest.sol";
import {Deployers} from "./utils/Deployers.sol";
import {GasSnapshot} from "forge-gas-snapshot/GasSnapshot.sol";
import {Currency, CurrencyLibrary} from "../src/types/Currency.sol";
import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";
import {Constants} from "../test/utils/Constants.sol";
import {SkipCallsTestHook} from "../src/test/SkipCallsTestHook.sol";

contract SkipCallsTest is Test, Deployers, GasSnapshot {
using PoolIdLibrary for PoolKey;

SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook(address(uint160(Hooks.BEFORE_SWAP_FLAG)));

function setUp() public {
SkipCallsTestHook impl = new SkipCallsTestHook();
vm.etch(address(skipCallsTestHook), address(impl).code);
deployFreshManagerAndRouters();
skipCallsTestHook.setManager(IPoolManager(manager));

(currency0, currency1) = deployMintAndApprove2Currencies();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just put all of this in setUp and then do the init/addLiq as needed in each of the test cases? I think it may be a bit easier to follow what exactly is being called to understand the expected count amount. And I dont think the hook address needs to change each time ie all of this can just happen once? Unless Im missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so make one hook and turn all flags on? and check that count is total 10 at the end?

(key,) = initPoolAndAddLiquidity(
currency0, currency1, IHooks(address(skipCallsTestHook)), 3000, SQRT_RATIO_1_1, ZERO_BYTES
);
}

function test_beforeSwap_skipIfCalledByHook() public {
IPoolManager.SwapParams memory swapParams =
IPoolManager.SwapParams({zeroForOne: true, amountSpecified: -100, sqrtPriceLimitX96: SQRT_RATIO_1_2});

PoolSwapTest.TestSettings memory testSettings =
PoolSwapTest.TestSettings({withdrawTokens: true, settleUsingTransfer: true, currencyAlreadySent: false});

MockERC20(Currency.unwrap(key.currency0)).approve(address(skipCallsTestHook), Constants.MAX_UINT256);

assertEq(skipCallsTestHook.counter(), 0);
swapRouter.swap(key, swapParams, testSettings, abi.encode(address(this)));
assertEq(skipCallsTestHook.counter(), 1);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a test where we call again on the pool and the counter should be 2 but not 4?

Loading