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 10 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 @@
309389
309451
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 @@
184232
184214
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
189549
189531
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 @@
125296
125216
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 @@
172155
172075
2 changes: 1 addition & 1 deletion .forge-snapshots/poolManager bytecode size.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
22909
22897
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 @@
96164
96146
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 @@
192350
192332
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
193814
193796
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 @@
176897
176806
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
190773
190682
Original file line number Diff line number Diff line change
@@ -1 +1 @@
112356
112265
2 changes: 1 addition & 1 deletion .forge-snapshots/swap against liquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
100359
100268
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 @@
120473
120382
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 @@
116428
116337
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 @@
182756
182665
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 @@
199565
199474
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 @@
133664
133573
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
100337
100246
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 @@
184666
184615
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
8 changes: 2 additions & 6 deletions src/test/PoolDonateTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,8 @@ contract PoolDonateTest is PoolTestBase {

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

(, uint256 poolBalanceBefore0, int256 deltaBefore0) =
_fetchBalances(data.key.currency0, data.sender, address(this));
(, uint256 poolBalanceBefore1, 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 0");
require(deltaBefore1 == 0, "deltaBefore1 is not 0");
Expand All @@ -58,8 +56,6 @@ contract PoolDonateTest is PoolTestBase {
(, uint256 poolBalanceAfter1, int256 deltaAfter1) =
_fetchBalances(data.key.currency1, data.sender, address(this));

require(poolBalanceBefore0 == poolBalanceAfter0, "poolBalanceBefore0 is not equal to poolBalanceAfter0");
require(poolBalanceBefore1 == poolBalanceAfter1, "poolBalanceBefore1 is not equal to poolBalanceAfter1");
require(deltaAfter0 == -int256(data.amount0), "deltaAfter0 is not equal to -int256(data.amount0)");
require(deltaAfter1 == -int256(data.amount1), "deltaAfter1 is not equal to -int256(data.amount1)");

Expand Down
15 changes: 4 additions & 11 deletions src/test/PoolSwapTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -52,23 +52,16 @@ contract PoolSwapTest is PoolTestBase {

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

(, uint256 poolBalanceBefore0, int256 deltaBefore0) =
_fetchBalances(data.key.currency0, data.sender, address(this));
(, uint256 poolBalanceBefore1, 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 poolBalanceAfter0, int256 deltaAfter0) =
_fetchBalances(data.key.currency0, data.sender, address(this));
(, uint256 poolBalanceAfter1, int256 deltaAfter1) =
_fetchBalances(data.key.currency1, data.sender, address(this));

require(poolBalanceBefore0 == poolBalanceAfter0, "poolBalanceBefore0 is not equal to poolBalanceAfter0");
require(poolBalanceBefore1 == poolBalanceAfter1, "poolBalanceBefore1 is not equal to poolBalanceAfter1");
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these being removed in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I think its because the hook might do a swap/add liq and settle that is nested. meaning the balance would have changed before the outermost swap settles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep exactly, the hook does its own swap which changes the balance

(,, 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
Loading
Loading