Allow Hook contract to call beforeInitialize and afterInitialize hooks

[tomwade]

Component

Pool Actions (swap, modifyLiquidity, donate, take, settle, mint)

Describe the suggested feature and problem it solves.

Last month a PR was added that prevented hook calls from being made when they were called from within the hook contract themselves:

8a4ebb0#diff-ff33f731a6fff1d6a1c4bdde67f734199c88a742966c3942a4fac5ab9be7c07fR99

This was then later slightly refactored in the PoolManager to create the modifier noSelfCall:

11aa1f5#diff-ff33f731a6fff1d6a1c4bdde67f734199c88a742966c3942a4fac5ab9be7c07fR106-R111

This means that the hook contract can no longer call its own functions.

I can see why this would be beneficial for swap calls and other functions, and would assume that this is to prevent recursive entry.

My suggestion, however, is that we remove the modifier from beforeInitialize and afterInitialize as the previous implementation allow for the hook contract to have control over who, how and when the pool could be initialized. I also don't see the harm in removing it from all functions, though I am more than happy to be proved wrong there.

This may be a selfish case, but I feel that platforms should have this level of control of initialization, if not across all functions.

Describe the desired implementation.

My desired implementation would simply be to remove the noSelfCall modifier from at least the initialization hooks.

Describe alternatives.

No response

Additional context.

No response

[linear]

PROTO-71 Allow Hook contract to call `beforeInitialize` and `afterInitialize` hooks

[snreynolds]

In what case would you want another call back to the hook? We see this as an optimization for when hooks already have control to finish executing anything before then finishing any further action on the pool manager, in which case there is no need to call back again into the hook

[tomwade]

So it is less that we are calling back into the hook and more that we are making the initial call to initialize() from a function inside the hook contract (not on a hook callback).

So as a bit of a code example that may be able to explain this better:

    /**
     * Once a collection has been registered via `registerCollection`, we can subsequently initialize it
     * by providing liquidity
     *
     * @dev Logic to ensure that valid tokens have been supplied should be enforced in the preceeding
     * {Locker} function that calls this.
     *
     * @dev This first liquidity position created will be full range to ensure that there is sufficient
     * liquidity provided.
     *
     * @param _collection The address of the collection being registered
     * @param _collectionToken The underlying ERC20 token of the collection
     * @param _tokens The number of underlying tokens being supplied
     * @param _sqrtPriceX96 The initial pool price
     */
    function initializeCollection(address _collection, ICollectionToken _collectionToken, uint _tokens, uint160 _sqrtPriceX96) internal override {
        // Ensure that the PoolKey is not empty
        PoolKey memory poolKey = _poolKeys[_collection];
        require(Currency.unwrap(poolKey.currency1) != address(0), 'Unknown collection');

        // Set transient storage to flag that we are permitted to initialize our pool
        assembly { tstore(_TSTORE_SLOT, 1) }

        // Initialise our pool
        poolManager.initialize(poolKey, _sqrtPriceX96, '');

        // Obtain the UV4 lock for the pool to pull in liquidity
        poolManager.unlock(
            abi.encode(CallbackData({
                poolKey: poolKey,
                donateAmount: 0,
                liquidityDelta: LiquidityAmounts.getLiquidityForAmounts({
                    sqrtRatioX96: _sqrtPriceX96,
                    sqrtRatioAX96: TickMath.getSqrtRatioAtTick(TickMath.minUsableTick(poolKey.tickSpacing)),
                    sqrtRatioBX96: TickMath.getSqrtRatioAtTick(TickMath.maxUsableTick(poolKey.tickSpacing)),
                    amount0: msg.value,
                    amount1: _tokens
                }),
                liquidityTokens: _tokens
            })
        ));
    }

We are registering a collection against an existing ERC721 and creating an underlying ERC20. We then want to ensure that the sqrtPriceX96 that is provided is not erroneous as this could essentially be initialized by anyone. We use the tstore to ensure that the beforeInitialize is triggered from our expected contract in this method.

We could bypass this issue by creating an external contract and giving this unique call rights in the beforeInitialize to call it, but this seems a little round-about.

This could potentially just be an incorrect approach to achieve our required logic, and happy to be lead down a better path, but it seems like preventing the hook contract from calling itself is potentially not what the initial PR intended to do, but instead wanting to prevent recursive hook calls?

[snreynolds]

If someone is initializing the pool through a function on your hook you should be able to do any necessary enforcement before initialize is called (meaning no callback is required & our optimization works for your case).

The alternative is that someone initializes the pool directly by calling initialize on the pool manager, in which case the hook would be triggered in beforeInitialize and you can then perform any extra checks.

Either case will work and in case 1) you don't need to re-enter the hook inside initialize because you should have already done any necessary checks before initialize is called on the pool manager.

Am I understanding your case correctly? If not please lmk what I'm missing!

[tomwade]

You were 100% right. I hadn't thought about that approach and maybe coded myself into a corner. I can just initialize directly and prevent external calls through beforeInitialize. I've closed the issue and thank you so much for your patience and great explanation!