v4-core icon indicating copy to clipboard operation
v4-core copied to clipboard

Published 20 hours ago • verified publisher icon Uniswap

Change Hook Flags to LSB

Open nishim3 opened this issue 10 months ago • 4 comments

Related Issue

Which issue does this pull request resolve? #553

Description of changes

nishim3 avatar Apr 04 '24 20:04 nishim3

Hey couple thoughts here - If we are going to change how hook address flags are set to enable leading 0s/vanity addresses we have been talking about another potential design that just removes the flags from the address entirely.

In this design, a hook address is any address with at least MAX_NUM_FLAG_PERMISSIONS of leading zeros. When we initialize a new pool with hooks and flags we just mask the flag bits in the pool key for the hook, and save the state according to the masked address. To call hook from pool key, we just zero out the leading flags.

This is better for a few reasons than current design because we'd be forcing leading zeros, and it would allow one hook contract to be associated with multiple pools and different permissions on each pool.

If we're going to change the design for hook flags, I think I'm definitely in favor of this design. Let me know thoughts cc @Philogy @nishim3

snreynolds avatar Apr 09 '24 05:04 snreynolds

@snreynolds I am ok with this implementation. Do let me know if and when this gets finalised.

nishim3 avatar Apr 09 '24 08:04 nishim3

@snreynolds If it matters at all, I very much like the approach of removing the bits from the address entirely and instead enforcing a minimum number of leading 0 bytes. Makes it much cleaner and allows for more optimization where necessary.

akshatmittal avatar Apr 17 '24 16:04 akshatmittal

Hey @snreynolds! Any update on the design?

nishim3 avatar May 05 '24 20:05 nishim3

Hey @snreynolds! Any update on the design?

Hey! I think we'd love to go forward with the original suggested design - moving the permissions to the lower bits. If you could update this PR to the base branch @hensha256 can give a review. Thanks!

snreynolds avatar May 17 '24 21:05 snreynolds

Hey @nishim3! Just pinging here to get main updated. Otherwise i will do so myself later!

hensha256 avatar May 20 '24 13:05 hensha256

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

socket-security[bot] avatar May 21 '24 22:05 socket-security[bot]

The tests are passing locally. Some issue with the lib folder is causing tests to fail here...Help appreciated

nishim3 avatar May 21 '24 22:05 nishim3