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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
1b71d99
skip calls to hooks when msg.sender is hook contract + test
dianakocsis Mar 11, 2024
cc849f5
move into callHook
dianakocsis Mar 12, 2024
5c7e31d
changes
dianakocsis Mar 15, 2024
87032f6
linting
dianakocsis Mar 15, 2024
f51464b
merge main into skip-calls-to-hooks
dianakocsis Mar 15, 2024
43c5d97
re-add forge snapshots
dianakocsis Mar 15, 2024
4f917fc
switch back to self instead of hook
dianakocsis Mar 20, 2024
cd9bff9
merge main
dianakocsis Mar 20, 2024
4bf9249
more tests
dianakocsis Mar 22, 2024
c956195
merge main
dianakocsis Mar 23, 2024
c399dc1
call again and assert counter is 2
dianakocsis Apr 3, 2024
5461d44
merge main
dianakocsis Apr 3, 2024
a55b9c1
make more clear
dianakocsis Apr 3, 2024
192694f
merge main
dianakocsis Apr 3, 2024
aeb2dae
clearAllHookPermisssionsMask
dianakocsis Apr 4, 2024
6a7251d
add comments and switch back to uint256 for now
dianakocsis Apr 5, 2024
6db559e
allowance does not get decremented if the sender address is the contr…
dianakocsis Apr 5, 2024
7941046
fix claims
dianakocsis Apr 5, 2024
632c546
remove import console.sol
dianakocsis Apr 5, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
265311
265373
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity with native token.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
140154
140136
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
145471
145453
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 1 token.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
101145
101049
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 2 tokens.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
132135
132039
2 changes: 1 addition & 1 deletion .forge-snapshots/poolManager bytecode size.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
22974
22962
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity with empty hook.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
56002
55984
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity with native token.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
148188
148170
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
149652
149634
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap with native.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
132905
132814
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
146781
146690
2 changes: 1 addition & 1 deletion .forge-snapshots/swap against liquidity with native token.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
72364
72273
2 changes: 1 addition & 1 deletion .forge-snapshots/swap against liquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
60367
60276
2 changes: 1 addition & 1 deletion .forge-snapshots/swap burn 6909 for input.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
80397
80306
2 changes: 1 addition & 1 deletion .forge-snapshots/swap burn native 6909 for input.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
76394
76303
2 changes: 1 addition & 1 deletion .forge-snapshots/swap mint native output as 6909.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
138745
138654
2 changes: 1 addition & 1 deletion .forge-snapshots/swap mint output as 6909.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
155596
155505
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with dynamic fee.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
89671
89580
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with hooks.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
60345
60254
2 changes: 1 addition & 1 deletion .forge-snapshots/update dynamic fee in before swap.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
140297
140246
25 changes: 13 additions & 12 deletions src/libraries/Hooks.sol
View file
Open in desktop
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
14 changes: 4 additions & 10 deletions src/test/PoolDonateTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,17 @@ 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");

BalanceDelta delta = manager.donate(data.key, data.amount0, data.amount1, 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));
(,, int256 deltaAfter0) = _fetchBalances(data.key.currency0, data.sender, address(this));
(,, 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
View file
Open in desktop
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