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 2 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 @@
312292
312352
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 @@
192474
192456
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
192452
192434
Original file line number Diff line number Diff line change
@@ -1 +1 @@
184412
184324
2 changes: 1 addition & 1 deletion .forge-snapshots/cached dynamic fee, no hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
138439
138312
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 @@
131592
131574
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 @@
176613
176595
2 changes: 1 addition & 1 deletion .forge-snapshots/poolManager bytecode size.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
23279
23263
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 @@
99307
99289
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 @@
200680
200662
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
196957
196939
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 @@
187012
186885
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
195549
195422
Original file line number Diff line number Diff line change
@@ -1 +1 @@
117675
117548
2 changes: 1 addition & 1 deletion .forge-snapshots/swap against liquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
105139
105012
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 @@
125350
125223
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 @@
121303
121176
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 @@
189538
189411
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 @@
206343
206216
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 @@
183513
183425
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
105117
104990
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 @@
190217
190129
24 changes: 13 additions & 11 deletions src/libraries/Hooks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,12 @@ library Hooks {

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

Choose a reason for hiding this comment

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

ooh this is an interesting line of code.. address(self) reads like address(this), but is actually checking address(hook)... I feel like I lean towards a rename of callHook(IHooks hook, bytes memory data)

Copy link
Member

Choose a reason for hiding this comment

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

could collapse the if statement by doing
if (msg.sender == address(hook) return;
_callHook()
if(selector != expectedSelector) revert ..

which may be clean? but try it out and see if you like it.. not that opinionated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I like that! Honestly it took me a while to figure out what was what so I think this will make it more clear

(bytes4 expectedSelector, bytes4 selector) = _callHook(self, data);

if (selector != expectedSelector) {
revert InvalidHookResponse();
if (selector != expectedSelector) {
revert InvalidHookResponse();
}
}
}

Expand Down Expand Up @@ -132,9 +134,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 +151,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 +166,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 +179,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 +188,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 +199,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
3 changes: 0 additions & 3 deletions src/test/PoolSwapTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,6 @@ contract PoolSwapTest is Test, PoolTestBase {
(,, uint256 reserveAfter0, int256 deltaAfter0) = _fetchBalances(data.key.currency0, data.sender, address(this));
(,, uint256 reserveAfter1, int256 deltaAfter1) = _fetchBalances(data.key.currency1, data.sender, address(this));

assertEq(reserveBefore0, reserveAfter0);
Copy link
Member

Choose a reason for hiding this comment

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

why do we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There could be a swap within the hook making the reserves not equal to each other so I removed it. Also since reservesOf might be removed entirely anyways

assertEq(reserveBefore1, reserveAfter1);

if (data.params.zeroForOne) {
if (data.params.amountSpecified > 0) {
// exact input, 0 for 1
Expand Down
59 changes: 59 additions & 0 deletions src/test/SkipCallsTestHook.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// 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 {FeeLibrary} from "../../src/libraries/FeeLibrary.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;
using FeeLibrary for uint24;

uint256 public counter;
IPoolManager manager;
uint24 internal fee;

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

function setFee(uint24 _fee) external {
Copy link
Member

Choose a reason for hiding this comment

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

These are unneeded functions i believe for the purpose of this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ya i needed them since I made the hook with a dynamic fee but ill change that so i dont need these

fee = _fee;
}

function getFee(address, PoolKey calldata) public view returns (uint24) {
return fee;
}

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(msg.sender).swap(key, params, hookData);
address payer = abi.decode(hookData, (address));
int256 delta0 = IPoolManager(msg.sender).currencyDelta(address(this), key.currency0);
dianakocsis marked this conversation as resolved.
Show resolved Hide resolved
assertEq(delta0, params.amountSpecified);
int256 delta1 = IPoolManager(msg.sender).currencyDelta(address(this), key.currency1);
assert(delta1 < 0);
IERC20Minimal(Currency.unwrap(key.currency0)).transferFrom(payer, msg.sender, uint256(delta0));
manager.settle(key.currency0);
manager.take(key.currency1, payer, uint256(-delta1));
}
}
70 changes: 70 additions & 0 deletions test/SkipCallsTestHook.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// 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 {FeeLibrary} from "../src/libraries/FeeLibrary.sol";
import {IPoolManager} from "../src/interfaces/IPoolManager.sol";
import {IFees} from "../src/interfaces/IFees.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 {IDynamicFeeManager} from "././../src/interfaces/IDynamicFeeManager.sol";
import {GasSnapshot} from "forge-gas-snapshot/GasSnapshot.sol";
import {DynamicFeesTestHook} from "../src/test/DynamicFeesTestHook.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;

/// 1111 1111 1100
SkipCallsTestHook skipCallsTestHook = SkipCallsTestHook(
address(
uint160(0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF)
& uint160(
~Hooks.BEFORE_INITIALIZE_FLAG & ~Hooks.AFTER_INITIALIZE_FLAG & ~Hooks.BEFORE_ADD_LIQUIDITY_FLAG
& ~Hooks.AFTER_ADD_LIQUIDITY_FLAG & ~Hooks.BEFORE_REMOVE_LIQUIDITY_FLAG
& ~Hooks.AFTER_REMOVE_LIQUIDITY_FLAG & ~Hooks.AFTER_SWAP_FLAG & ~Hooks.BEFORE_DONATE_FLAG
& ~Hooks.AFTER_DONATE_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)),
FeeLibrary.DYNAMIC_FEE_FLAG,
dianakocsis marked this conversation as resolved.
Show resolved Hide resolved
SQRT_RATIO_1_1,
ZERO_BYTES
);
}

function test_beforeSwap_invalidSender() public {
Copy link
Member

Choose a reason for hiding this comment

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

what do you mean by invalid_sender in the test name?

I think this is just testing that the hook's call to beforeSwap doesnt trigger the hook again yes? thus, the count to beforeSwap is 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking more like invalid caller aka the hook, but ill rename it to test_beforeSwap_skipIfCalledByHook() to make it more clear

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