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

dianakocsis · 2024-03-11T22:16:32Z

Related Issue

Skip calls to hooks when msg.sender is the hook contract #471

Description of changes

skips calls to hooks when msg.sender is the hook contract + tests

src/libraries/Hooks.sol

src/test/SkipCallsTestHook.sol

snreynolds · 2024-03-14T23:46:01Z

src/libraries/Hooks.sol

@@ -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)) {


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)

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

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

snreynolds · 2024-03-14T23:48:51Z

src/test/PoolSwapTest.sol

@@ -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);


why do we remove this?

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

src/test/SkipCallsTestHook.sol

snreynolds · 2024-03-14T23:55:34Z

src/test/SkipCallsTestHook.sol

+        manager = _manager;
+    }
+
+    function setFee(uint24 _fee) external {


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

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

test/SkipCallsTestHook.t.sol

snreynolds · 2024-03-14T23:59:13Z

test/SkipCallsTestHook.t.sol

+        );
+    }
+
+    function test_beforeSwap_invalidSender() public {


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

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

snreynolds

changes look good. @hensha256 brought up a good point that changing away from self diverges from how we do this in other libs (Pool.sol, Position.sol) so I'm actually inclined to change it back... and we can discuss more later. Sorry for back and forth there

snreynolds · 2024-03-25T21:00:11Z

test/SkipCallsTestHook.t.sol

+        SkipCallsTestHook impl = new SkipCallsTestHook();
+        vm.etch(address(skipCallsTestHook), address(impl).code);
+        deployFreshManagerAndRouters();
+        skipCallsTestHook.setManager(IPoolManager(manager));
+
+        (currency0, currency1) = deployMintAndApprove2Currencies();


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

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

snreynolds · 2024-03-25T21:04:55Z

src/test/SkipCallsTestHook.sol

+        bytes calldata hookData
+    ) external override returns (bytes4) {
+        counter++;
+        _initialize(key, Constants.SQRT_RATIO_1_1, hookData);


Maybe overkill but it seems like we are just calling the same function on the pool again (except for this.. this one does initialize).... Maybe instead we actually fuzz the function thats called inside? That way we are testing that no matter which hook callback we are in, any action on the pool called from the hook skips the call? So it could be any of initialize/swap/modifyPositon/donate

snreynolds · 2024-03-25T21:05:55Z

test/SkipCallsTestHook.t.sol

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


assert counter is 0 before the call in each of these? that makes it super clear we are expecting a change to counter.. I think you do it below but not here/ i think its missing in some other tests as well

snreynolds · 2024-03-25T21:06:18Z

test/SkipCallsTestHook.t.sol

+        donateRouter.donate(key, 100, 200, abi.encode(address(this)));
+        assertEq(skipCallsTestHook.counter(), 1);
+    }
+}


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

github-actions · 2024-04-03T17:17:09Z

Forge code coverage:

File	% Lines	% Statements	% Branches	% Funcs
src/ERC6909.sol	91.30% (21/23)	85.71% (24/28)	100.00% (4/4)	85.71% (6/7)
src/ERC6909Claims.sol	100.00% (6/6)	100.00% (8/8)	100.00% (4/4)	100.00% (1/1)
src/NoDelegateCall.sol	100.00% (1/1)	100.00% (3/3)	100.00% (2/2)	100.00% (1/1)
src/PoolManager.sol	90.91% (80/88)	89.34% (109/122)	96.67% (29/30)	73.08% (19/26)
src/ProtocolFees.sol	100.00% (13/13)	95.65% (22/23)	87.50% (7/8)	100.00% (3/3)
src/libraries/BitMath.sol	100.00% (47/47)	100.00% (57/57)	100.00% (36/36)	100.00% (2/2)
src/libraries/CurrencyDelta.sol	100.00% (5/5)	100.00% (7/7)	100.00% (0/0)	100.00% (3/3)
src/libraries/FullMath.sol	100.00% (29/29)	100.00% (33/33)	100.00% (8/8)	100.00% (2/2)
src/libraries/Hooks.sol	100.00% (42/42)	98.73% (78/79)	96.67% (29/30)	100.00% (14/14)
src/libraries/Lock.sol	100.00% (4/4)	100.00% (4/4)	100.00% (0/0)	100.00% (3/3)
src/libraries/NonZeroDeltaCount.sol	100.00% (6/6)	100.00% (6/6)	100.00% (0/0)	100.00% (3/3)
src/libraries/Pool.sol	99.23% (129/130)	97.18% (138/142)	93.02% (80/86)	100.00% (13/13)
src/libraries/PoolGetters.sol	100.00% (3/3)	100.00% (3/3)	100.00% (0/0)	100.00% (3/3)
src/libraries/Position.sol	100.00% (12/12)	100.00% (14/14)	100.00% (6/6)	100.00% (2/2)
src/libraries/ProtocolFeeLibrary.sol	100.00% (8/8)	100.00% (12/12)	100.00% (4/4)	100.00% (3/3)
src/libraries/SafeCast.sol	100.00% (8/8)	100.00% (14/14)	100.00% (8/8)	100.00% (4/4)
src/libraries/SqrtPriceMath.sol	100.00% (32/32)	98.48% (65/66)	83.33% (20/24)	100.00% (8/8)
src/libraries/SwapFeeLibrary.sol	100.00% (5/5)	100.00% (9/9)	100.00% (4/4)	100.00% (3/3)
src/libraries/SwapMath.sol	100.00% (23/23)	100.00% (30/30)	100.00% (12/12)	100.00% (1/1)
src/libraries/TickBitmap.sol	100.00% (19/19)	100.00% (34/34)	100.00% (6/6)	100.00% (3/3)
src/libraries/TickMath.sol	97.87% (92/94)	97.30% (144/148)	97.83% (45/46)	50.00% (2/4)
src/libraries/UnsafeMath.sol	100.00% (1/1)	100.00% (1/1)	100.00% (0/0)	100.00% (1/1)
src/types/BalanceDelta.sol	100.00% (2/2)	100.00% (2/2)	100.00% (0/0)	100.00% (2/2)
src/types/Currency.sol	100.00% (15/15)	91.67% (22/24)	80.00% (8/10)	100.00% (6/6)
src/types/PoolId.sol	100.00% (1/1)	100.00% (1/1)	100.00% (0/0)	100.00% (1/1)
Total	77.46% (1041/1344)	77.02% (1411/1832)	56.18% (418/744)	66.45% (202/304)

hensha256

generally looking great!! couple of small things but we're nearly there

hensha256 · 2024-04-04T21:19:36Z

src/test/PoolSwapTest.sol

-            _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");


why are these being removed in this PR?

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

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

hensha256 · 2024-04-04T21:25:18Z

test/SkipCallsTestHook.t.sol

+                    & uint160(
+                        ~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.BEFORE_SWAP_FLAG
+                            & ~Hooks.AFTER_SWAP_FLAG & ~Hooks.BEFORE_DONATE_FLAG & ~Hooks.AFTER_DONATE_FLAG


so these lines can become

address( uint160( uint256(type(uint160).max) & clearAllHookPermisssionsMask | Hooks.AFTER_INITIALIZE_FLAG ) )

not sure why it needs to go via uint256... you could try making clearAllHookPermisssionsMask a uint160 too

i think you commented something similar in #548 which I can do over there

ah so much nicer! thank you!!

test/SkipCallsTestHook.t.sol

hensha256 · 2024-04-04T21:29:31Z

test/SkipCallsTestHook.t.sol

+        deploy(skipCallsTestHook);
+        approveAndAddLiquidity(skipCallsTestHook);
+
+        modifyLiquidityRouter.modifyLiquidity(key, REMOVE_LIQ_PARAMS, abi.encode(address(this)));


assertEq(skipCallsTestHook.counter(), 0);

above this line too

test/SkipCallsTestHook.t.sol

…act address

hensha256 · 2024-04-05T18:00:55Z

test/ERC6909Claims.t.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.15;
 import {Test} from "forge-std/Test.sol";
 import {CurrencyLibrary, Currency} from "../src/types/Currency.sol";
 import {MockERC6909Claims} from "../src/test/MockERC6909Claims.sol";
+import "forge-std/console.sol";


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

1b71d99

nishim3 mentioned this pull request Mar 12, 2024

Skip calls to hooks when msg.sender is the hook contract #502

Closed

hensha256 reviewed Mar 12, 2024

View reviewed changes

src/libraries/Hooks.sol Outdated Show resolved Hide resolved

hensha256 reviewed Mar 12, 2024

View reviewed changes

src/test/SkipCallsTestHook.sol Outdated Show resolved Hide resolved

hensha256 reviewed Mar 12, 2024

View reviewed changes

src/test/SkipCallsTestHook.sol Outdated Show resolved Hide resolved

move into callHook

cc849f5

snreynolds requested changes Mar 14, 2024

View reviewed changes

dianakocsis added 4 commits March 15, 2024 10:54

changes

5c7e31d

linting

87032f6

merge main into skip-calls-to-hooks

f51464b

re-add forge snapshots

43c5d97

snreynolds self-requested a review March 20, 2024 18:48

snreynolds reviewed Mar 20, 2024

View reviewed changes

dianakocsis added 2 commits March 20, 2024 16:52

switch back to self instead of hook

4f917fc

merge main

cd9bff9

snreynolds previously approved these changes Mar 20, 2024

View reviewed changes

more tests

4bf9249

dianakocsis dismissed snreynolds’s stale review via 4bf9249 March 22, 2024 20:07

merge main

c956195

snreynolds requested changes Mar 25, 2024

View reviewed changes

dianakocsis added 2 commits April 3, 2024 13:14

call again and assert counter is 2

c399dc1

merge main

5461d44

dianakocsis added 2 commits April 3, 2024 15:37

make more clear

a55b9c1

merge main

192694f

dianakocsis requested review from snreynolds and hensha256 April 3, 2024 19:39

hensha256 requested changes Apr 4, 2024

View reviewed changes

clearAllHookPermisssionsMask

aeb2dae

snreynolds requested changes Apr 5, 2024

View reviewed changes

test/SkipCallsTestHook.t.sol Show resolved Hide resolved

dianakocsis added 3 commits April 5, 2024 11:27

add comments and switch back to uint256 for now

6a7251d

allowance does not get decremented if the sender address is the contr…

6db559e

…act address

fix claims

7941046

dianakocsis requested review from snreynolds and hensha256 April 5, 2024 17:23

hensha256 reviewed Apr 5, 2024

View reviewed changes

remove import console.sol

632c546

hensha256 approved these changes Apr 5, 2024

View reviewed changes

hensha256 mentioned this pull request Apr 5, 2024

Allow max int value on max liquidity check #563

Merged

snreynolds approved these changes Apr 5, 2024

View reviewed changes

dianakocsis merged commit 8a4ebb0 into main Apr 5, 2024
6 checks passed

dianakocsis deleted the skip-calls-to-hooks branch April 5, 2024 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dianakocsis commented Mar 11, 2024

snreynolds Mar 14, 2024

snreynolds Mar 14, 2024

dianakocsis Mar 15, 2024

snreynolds Mar 14, 2024

dianakocsis Mar 15, 2024

snreynolds Mar 14, 2024

dianakocsis Mar 15, 2024

snreynolds Mar 14, 2024

dianakocsis Mar 15, 2024

snreynolds left a comment

snreynolds Mar 25, 2024

dianakocsis Mar 25, 2024

snreynolds Mar 25, 2024

snreynolds Mar 25, 2024

snreynolds Mar 25, 2024

github-actions bot commented Apr 3, 2024 •

edited

Loading

hensha256 left a comment

hensha256 Apr 4, 2024

snreynolds Apr 5, 2024

dianakocsis Apr 5, 2024

hensha256 Apr 4, 2024

hensha256 Apr 4, 2024

snreynolds Apr 5, 2024

dianakocsis Apr 5, 2024

hensha256 Apr 4, 2024

hensha256 Apr 5, 2024

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

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

Conversation

dianakocsis commented Mar 11, 2024

Related Issue

Description of changes

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snreynolds left a 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

Choose a reason for hiding this comment

github-actions bot commented Apr 3, 2024 • edited Loading

hensha256 left a 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

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

github-actions bot commented Apr 3, 2024 •

edited

Loading