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

Split {before,after}ModifyPosition into {before,after}Mint and {before,after}Burn #350

Closed
danrobinson opened this issue Sep 5, 2023 · 0 comments · Fixed by #434
Closed
Assignees
Labels
p0 Very important to fix

Comments

@danrobinson
Copy link
Contributor

Component

Hooks

Describe the suggested feature and problem it solves.

Right now, beforeModifyPosition is a risky permission. Liquidity providers need to audit and trust the hook code or risk never being able to withdraw their money. That makes it harder to fearlessly integrate pools that have this permission.

However, the vast majority of that risk is from the ability of the hook to interrupt burns, rather than mints. beforeMint would generally be a more harmless hook, since it would never prevent existing liquidity providers from withdrawing their money. If we split out the modifyPosition hooks into ones that affect only mints and burns respectively, we allow hooks that only need to be called on mints to avoid asking for dangerous permissions that they don't need.

Describe the desired implementation.

Replace the hook calls in https://github.com/Uniswap/v4-core/blob/main/contracts/PoolManager.sol#L205 and https://github.com/Uniswap/v4-core/blob/main/contracts/PoolManager.sol#L243C1-L251C1 with switch statements that call the appropriate beforeMint/beforeBurn/afterMint/afterBurn based on whether params.liquidityDelta is positive or negative.

Describe alternatives.

Hook contracts could use a switch statement at the beginning of their callback implementation to make it easy to see that they will never interfere with burn transactions.

But this generally undermines the point of hook permissions. It still requires reading the code of the contract and can't be verified programmatically the way hook permissions can.

And it would be trickier for hook contracts that are upgradeable (which would have to put that check in the proxy contract somehow).

Additional context.

No response

@danrobinson danrobinson changed the title Explore splitting {before,after}ModifyPosition into {before,after}Mint and {before,after}Burn Split {before,after}ModifyPosition into {before,after}Mint and {before,after}Burn Sep 5, 2023
@snreynolds snreynolds added p0 Very important to fix and removed triage labels Oct 12, 2023
@willpote willpote assigned zhongeric and unassigned rileydcampbell Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p0 Very important to fix
Projects
None yet
4 participants