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

NoOp implementation with flag #324

Merged
merged 31 commits into from
Nov 22, 2023
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
f8f4a2f
cherrypicking
thogard785 Jun 30, 2023
eb91bfe
tests running and update snaps
emmaguo13 Jul 28, 2023
33138c5
test supoprt
emmaguo13 Jul 28, 2023
aa8d988
format and run hardhat tests
emmaguo13 Jul 28, 2023
646034f
remove irrelevant comment
emmaguo13 Jul 28, 2023
311d086
test noops
emmaguo13 Aug 3, 2023
7599fd9
add tests for noop
emmaguo13 Aug 3, 2023
6e3c350
merge with main
emmaguo13 Aug 9, 2023
68a42b3
lint and snapshots
emmaguo13 Aug 9, 2023
51c8afc
fix hook tests
emmaguo13 Aug 10, 2023
06659bb
clean
emmaguo13 Aug 10, 2023
e84f739
Merge branch 'main' into noop
hensha256 Nov 15, 2023
5251f35
foundry toml
hensha256 Nov 15, 2023
98fefe8
helper function
hensha256 Nov 15, 2023
ba21f75
Revert early if pool isnt initialized
hensha256 Nov 15, 2023
63965f9
Merge branch 'main' into noop
hensha256 Nov 15, 2023
074fcec
Extra tests
hensha256 Nov 15, 2023
feff1ad
Merge branch 'main' into noop
hensha256 Nov 15, 2023
9a2e0d2
Merge branch 'main' into noop
hensha256 Nov 17, 2023
f0c8226
Tests NoOps on disallowed hooks fail
hensha256 Nov 17, 2023
6c35c7e
linting
hensha256 Nov 17, 2023
c9ce471
snapshots
hensha256 Nov 17, 2023
7273540
helper function for initialized pool
hensha256 Nov 20, 2023
24eb878
PR comments
hensha256 Nov 20, 2023
2267508
PR comment test coverage
hensha256 Nov 20, 2023
90647a3
Final PR comments
hensha256 Nov 20, 2023
f70a8f5
comments about sentinel value
hensha256 Nov 22, 2023
3f0ebcb
Merge branch 'main' into noop
hensha256 Nov 22, 2023
bcfe08b
Fix masking tests
hensha256 Nov 22, 2023
dd3e523
decrease calls to assume
hensha256 Nov 22, 2023
06c85b8
Merge branch 'main' into noop
hensha256 Nov 22, 2023
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
Original file line number Diff line number Diff line change
@@ -1 +1 @@
189798
189980
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 @@
145314
145476
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 @@
134915
137124
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 @@
186636
186845
2 changes: 1 addition & 1 deletion .forge-snapshots/initialize.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
53843
53941
2 changes: 1 addition & 1 deletion .forge-snapshots/mint with empty hook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
319184
319291
2 changes: 1 addition & 1 deletion .forge-snapshots/mint with native token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
198926
199013
2 changes: 1 addition & 1 deletion .forge-snapshots/mint.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
202041
202128
1 change: 1 addition & 0 deletions .forge-snapshots/modify position with noop.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
52263
2 changes: 1 addition & 1 deletion .forge-snapshots/poolManager bytecode size.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24805
25025
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap native.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
132772
132954
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 @@
190722
190884
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
202425
202587
Original file line number Diff line number Diff line change
@@ -1 +1 @@
121318
121480
2 changes: 1 addition & 1 deletion .forge-snapshots/swap against liquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
109143
109305
2 changes: 1 addition & 1 deletion .forge-snapshots/swap burn claim for input.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
127882
128044
2 changes: 1 addition & 1 deletion .forge-snapshots/swap mint output as claim.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
212594
212756
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 @@
189038
189220
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
109122
109284
1 change: 1 addition & 0 deletions .forge-snapshots/swap with native.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
47418
1 change: 1 addition & 0 deletions .forge-snapshots/swap with noop.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
44944
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 @@
195628
195804
221 changes: 113 additions & 108 deletions .gas-snapshot

Large diffs are not rendered by default.

44 changes: 31 additions & 13 deletions src/PoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -185,16 +185,20 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
IPoolManager.ModifyPositionParams memory params,
bytes calldata hookData
) external override noDelegateCall onlyByLocker returns (BalanceDelta delta) {
PoolId id = key.toId();
Copy link
Member

Choose a reason for hiding this comment

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

Why move this to the top level? Seems cleaner to have it in Pool.sol

Copy link
Contributor

Choose a reason for hiding this comment

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

Because if its in the pool, then its after the hook call. So that means on an uninitialized pool, a swap transaction succeeds with a hook that NoOps (even though the pool doesnt exist). So I've moved it top level so that we can maintain the invariant that actions on an uninitialized pool always revert

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe new modifier onlyInitializedPool(key)? that duplicates id hashing in code but id guess it gets compiled out, could check?

Copy link
Member

Choose a reason for hiding this comment

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

modifier or helper func sgtm :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Modifier put gas up by 1000 for swaps because of repeated hashing. I've put in a helper function instead

if (pools[id].isNotInitialized()) revert PoolNotInitialized();

if (key.hooks.shouldCallBeforeModifyPosition()) {
if (
key.hooks.beforeModifyPosition(msg.sender, key, params, hookData)
!= IHooks.beforeModifyPosition.selector
) {
revert Hooks.InvalidHookResponse();
bytes4 selector = key.hooks.beforeModifyPosition(msg.sender, key, params, hookData);
hensha256 marked this conversation as resolved.
Show resolved Hide resolved
if (selector != IHooks.beforeModifyPosition.selector) {
if (key.hooks.isValidNoOpCall(selector)) {
hensha256 marked this conversation as resolved.
Show resolved Hide resolved
return BalanceDelta.wrap(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe some return indication here to tell the caller that custom accounting was used?
Either a special sentinel value (though that could probs always conflict with a truly possible balancedelta) or a separate boolean?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm interesting.. I like that idea. That would definitely make writing tests (or any integrating contract) a little easier so I'm in favor.

Copy link
Contributor

@hensha256 hensha256 Nov 20, 2023

Choose a reason for hiding this comment

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

Changed it to return the maximum uint256... it feels unlikely that a legit trade would ever return that?

} else {
revert Hooks.InvalidHookResponse();
}
}
}

PoolId id = key.toId();
Pool.FeeAmounts memory feeAmounts;
(delta, feeAmounts) = pools[id].modifyPosition(
Pool.ModifyPositionParams({
Expand Down Expand Up @@ -243,14 +247,20 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
onlyByLocker
returns (BalanceDelta delta)
{
PoolId id = key.toId();
if (pools[id].isNotInitialized()) revert PoolNotInitialized();

if (key.hooks.shouldCallBeforeSwap()) {
if (key.hooks.beforeSwap(msg.sender, key, params, hookData) != IHooks.beforeSwap.selector) {
revert Hooks.InvalidHookResponse();
bytes4 selector = key.hooks.beforeSwap(msg.sender, key, params, hookData);
if (selector != IHooks.beforeSwap.selector) {
if (key.hooks.isValidNoOpCall(selector)) {
return BalanceDelta.wrap(0);
} else {
revert Hooks.InvalidHookResponse();
}
}
}

PoolId id = key.toId();

uint256 feeForProtocol;
uint256 feeForHook;
uint24 swapFee;
Expand Down Expand Up @@ -295,13 +305,21 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
onlyByLocker
returns (BalanceDelta delta)
{
PoolId id = key.toId();
if (pools[id].isNotInitialized()) revert PoolNotInitialized();

if (key.hooks.shouldCallBeforeDonate()) {
if (key.hooks.beforeDonate(msg.sender, key, amount0, amount1, hookData) != IHooks.beforeDonate.selector) {
revert Hooks.InvalidHookResponse();
bytes4 selector = key.hooks.beforeDonate(msg.sender, key, amount0, amount1, hookData);
if (selector != IHooks.beforeDonate.selector) {
if (key.hooks.isValidNoOpCall(selector)) {
return BalanceDelta.wrap(0);
} else {
revert Hooks.InvalidHookResponse();
}
}
}

delta = _getPool(key).donate(amount0, amount1);
hensha256 marked this conversation as resolved.
Show resolved Hide resolved
delta = pools[id].donate(amount0, amount1);

_accountPoolBalanceDelta(key, delta);

Expand Down
3 changes: 3 additions & 0 deletions src/interfaces/IPoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ interface IPoolManager is IFees, IClaims {
/// @notice Thrown when a currency is not netted out after a lock
error CurrencyNotSettled();

/// @notice Thrown when trying to interact with a non-initialized pool
error PoolNotInitialized();

/// @notice Thrown when a function is called by an address that is not the current locker
/// @param locker The current locker
error LockedBy(address locker);
Expand Down
20 changes: 19 additions & 1 deletion src/libraries/Hooks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ library Hooks {
uint256 internal constant AFTER_SWAP_FLAG = 1 << 154;
uint256 internal constant BEFORE_DONATE_FLAG = 1 << 153;
uint256 internal constant AFTER_DONATE_FLAG = 1 << 152;
uint256 internal constant NO_OP_FLAG = 1 << 151;

bytes4 public constant NO_OP_SELECTOR = bytes4(keccak256(abi.encodePacked("NoOp")));

struct Calls {
bool beforeInitialize;
Expand All @@ -29,6 +32,7 @@ library Hooks {
bool afterSwap;
bool beforeDonate;
bool afterDonate;
bool noOp;
}

/// @notice Thrown if the address will not lead to the specified hook calls being called
Expand All @@ -50,6 +54,7 @@ library Hooks {
|| calls.afterModifyPosition != shouldCallAfterModifyPosition(self)
|| calls.beforeSwap != shouldCallBeforeSwap(self) || calls.afterSwap != shouldCallAfterSwap(self)
|| calls.beforeDonate != shouldCallBeforeDonate(self) || calls.afterDonate != shouldCallAfterDonate(self)
|| calls.noOp != shouldAllowNoOp(self)
) {
revert HookAddressNotValid(address(self));
}
Expand All @@ -58,7 +63,12 @@ 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) {
// If there is no hook contract set, then fee cannot be dynamic and there cannot be a hook fee on swap or withdrawal.
hensha256 marked this conversation as resolved.
Show resolved Hide resolved
hensha256 marked this conversation as resolved.
Show resolved Hide resolved
if (
hensha256 marked this conversation as resolved.
Show resolved Hide resolved
shouldAllowNoOp(hook) && !shouldCallBeforeModifyPosition(hook) && !shouldCallBeforeSwap(hook)
&& !shouldCallBeforeDonate(hook)
) {
return false;
}
return address(hook) == address(0)
? !fee.isDynamicFee() && !fee.hasHookSwapFee() && !fee.hasHookWithdrawFee()
: (
Expand Down Expand Up @@ -98,4 +108,12 @@ library Hooks {
function shouldCallAfterDonate(IHooks self) internal pure returns (bool) {
return uint256(uint160(address(self))) & AFTER_DONATE_FLAG != 0;
}

function shouldAllowNoOp(IHooks self) internal pure returns (bool) {
hensha256 marked this conversation as resolved.
Show resolved Hide resolved
return uint256(uint160(address(self))) & NO_OP_FLAG != 0;
}

function isValidNoOpCall(IHooks self, bytes4 selector) internal pure returns (bool) {
return shouldAllowNoOp(self) && selector == NO_OP_SELECTOR;
}
}
22 changes: 13 additions & 9 deletions src/libraries/Pool.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ library Pool {
using TickBitmap for mapping(int16 => uint256);
using Position for mapping(bytes32 => Position.Info);
using Position for Position.Info;
using Pool for State;

/// @notice Thrown when tickLower is not below tickUpper
/// @param tickLower The invalid tickLower
Expand Down Expand Up @@ -134,20 +135,20 @@ library Pool {
}

function setProtocolFees(State storage self, uint24 protocolFees) internal {
if (self.slot0.sqrtPriceX96 == 0) revert PoolNotInitialized();
if (self.isNotInitialized()) revert PoolNotInitialized();

self.slot0.protocolFees = protocolFees;
}

function setHookFees(State storage self, uint24 hookFees) internal {
if (self.slot0.sqrtPriceX96 == 0) revert PoolNotInitialized();
if (self.isNotInitialized()) revert PoolNotInitialized();

self.slot0.hookFees = hookFees;
}

/// @notice Only dynamic fee pools may update the swap fee.
function setSwapFee(State storage self, uint24 swapFee) internal {
if (self.slot0.sqrtPriceX96 == 0) revert PoolNotInitialized();
if (self.isNotInitialized()) revert PoolNotInitialized();
self.slot0.swapFee = swapFee;
}

Expand Down Expand Up @@ -179,23 +180,22 @@ library Pool {
uint256 feeForHook1;
}

/// @dev Effect changes to a position in a pool
/// @notice Effect changes to a position in a pool
/// @dev PoolManager checks that the pool is initialized before calling
/// @param params the position details and the change to the position's liquidity to effect
/// @return result the deltas of the token balances of the pool
function modifyPosition(State storage self, ModifyPositionParams memory params)
internal
returns (BalanceDelta result, FeeAmounts memory fees)
{
if (self.slot0.sqrtPriceX96 == 0) revert PoolNotInitialized();

checkTicks(params.tickLower, params.tickUpper);

uint256 feesOwed0;
uint256 feesOwed1;
{
ModifyPositionState memory state;
// if we need to update the ticks, do it

// if we need to update the ticks, do it
if (params.liquidityDelta != 0) {
(state.flippedLower, state.liquidityGrossAfterLower) =
updateTick(self, params.tickLower, params.liquidityDelta, false);
Expand Down Expand Up @@ -386,7 +386,8 @@ library Pool {
uint160 sqrtPriceLimitX96;
}

/// @dev Executes a swap against the state, and returns the amount deltas of the pool
/// @notice Executes a swap against the state, and returns the amount deltas of the pool
/// @dev PoolManager checks that the pool is initialized before calling
function swap(State storage self, SwapParams memory params)
internal
returns (
Expand All @@ -401,7 +402,6 @@ library Pool {

Slot0 memory slot0Start = self.slot0;
swapFee = slot0Start.swapFee;
if (slot0Start.sqrtPriceX96 == 0) revert PoolNotInitialized();
if (params.zeroForOne) {
if (params.sqrtPriceLimitX96 >= slot0Start.sqrtPriceX96) {
revert PriceLimitAlreadyExceeded(slot0Start.sqrtPriceX96, params.sqrtPriceLimitX96);
Expand Down Expand Up @@ -681,6 +681,10 @@ library Pool {
}
}

function isNotInitialized(State storage self) internal view returns (bool) {
return self.slot0.sqrtPriceX96 == 0;
}

/// @notice Clears tick data
/// @param self The mapping containing all initialized tick information for initialized ticks
/// @param tick The tick that will be cleared
Expand Down
3 changes: 2 additions & 1 deletion src/test/EmptyTestHooks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ contract EmptyTestHooks is IHooks {
beforeSwap: true,
afterSwap: true,
beforeDonate: true,
afterDonate: true
afterDonate: true,
noOp: true
})
);
}
Expand Down
54 changes: 54 additions & 0 deletions src/test/NoOpTestHooks.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {BaseTestHooks} from "./BaseTestHooks.sol";
import {Hooks} from "../libraries/Hooks.sol";
import {IPoolManager} from "../interfaces/IPoolManager.sol";
import {PoolKey} from "../types/PoolKey.sol";
import {BalanceDelta} from "../types/BalanceDelta.sol";

contract NoOpTestHooks is BaseTestHooks {
constructor() {
Hooks.validateHookAddress(
this,
Hooks.Calls({
beforeInitialize: false,
afterInitialize: false,
beforeModifyPosition: true,
afterModifyPosition: false,
beforeSwap: true,
afterSwap: false,
beforeDonate: true,
afterDonate: false,
noOp: true
})
);
}

function beforeModifyPosition(address, PoolKey calldata, IPoolManager.ModifyPositionParams calldata, bytes calldata)
external
pure
override
returns (bytes4)
{
return Hooks.NO_OP_SELECTOR;
}

function beforeSwap(address, PoolKey calldata, IPoolManager.SwapParams calldata, bytes calldata)
external
pure
override
returns (bytes4)
{
return Hooks.NO_OP_SELECTOR;
}

function beforeDonate(address, PoolKey calldata, uint256, uint256, bytes calldata)
external
pure
override
returns (bytes4)
{
return Hooks.NO_OP_SELECTOR;
}
}
Loading