feat: add reverse mapping for pool key, store tL, tU #310

snreynolds · 2024-08-28T02:24:34Z

motivation was to have better interface standardization for ERC721s
(functions dont require PositionConfig to be passed in)

& you get an onchain lookup of poolKey, tickLower, tickUpper
& position descriptor can read from config

I personally was against this for gas reasons (mint is 40-50K more expensive) but that should go down with mints on the same pool, and overhead for other ops is around 4-5K.

.forge-snapshots/PositionManager_mint_native.snap

hensha256 · 2024-08-28T16:10:24Z

(mint is 40-50K more expensive

so its like 8k more for mints on the same pool right? which is the "normal" case?

src/libraries/PositionConfig.sol

src/interfaces/ISubscriber.sol

src/PositionManager.sol

snreynolds · 2024-08-28T17:20:55Z

(mint is 40-50K more expensive

so its like 8k more for mints on the same pool right? which is the "normal" case?

not sure I can add a test but sounds about right it definitely wont be 40-50K

saucepoint

despite the massive diff, i quite like this

something about not checking subscriber on mint feels nice, same with maybe making burn only call unsubscribe

src/PositionManager.sol

src/libraries/PositionInfoLibrary.sol

test/erc721Permit/ERC721Permit.permit.t.sol

saucepoint · 2024-08-29T03:05:44Z

src/PositionManager.sol

        uint256 liquidity,
        uint128 amount0Max,
        uint128 amount1Max,
        bytes calldata hookData
-    ) internal onlyIfApproved(msgSender(), tokenId) onlyValidConfig(tokenId, config) {
+    ) internal onlyIfApproved(msgSender(), tokenId) {
+        (PositionInfo memory info, PoolKey memory poolKey) = getPoolPositionInfo(tokenId);


are there gas savings if we dont load the entirety of PositionInfo? we're not using the truncated poolId obviously, and the ticks are just repacked to IPoolManager.ModifyLiquidityParams

something like this could be quite cool

( PoolKey memory key, IPoolManager.ModifyLiquidityParams memory liquidityParams, bool hasSubscriber ) = veryAwesomeFunction(tokenId); liquidityParams.liquidityDelta = liquidityChange;

theres also ways to pack it further since PoolKey is 528 bits, so we can fit tL/tU/hasSubscriber in the remaining bits of the 3rd word

yeah was thinking the same thing.. I think I'm actually happy with it now that I have a version where the PositionInfo is packed (uint256).. since its the minimum we can basically return

snreynolds · 2024-08-29T04:07:05Z

src/libraries/PositionConfig.sol

@@ -3,33 +3,9 @@ pragma solidity ^0.8.24;

 import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol";

-// A PositionConfig is the input for creating and modifying a Position in core, whose truncated hash is set per tokenId
+// A PositionConfig is the input for creating and modifying a Position in core


Keeping this for now to reduce merge conflicts

later can move to test utils

woudl rather delete now that we arent using it

src/base/Notifier.sol

src/PositionManager.sol

hensha256 · 2024-08-29T12:08:08Z

src/PositionManager.sol

-    function _positionConfigs(uint256 tokenId) internal view override returns (PositionConfigId storage) {
-        return positionConfigs[tokenId];
-    }
+    mapping(uint256 tokenId => PackedPositionInfo info) public positionInfo;


now that its packed maybe worth having this internal and exposing a getter that returns the elements individually?

hm I dont feel strongly about it. For offchain, a simple sdk helper can dissect it. and onchain someone can just import the PositionInfoLibrary?

But if you prefer I can add

src/libraries/PoolKeyChecker.sol

src/PositionManager.sol

src/libraries/PositionInfoLibrary.sol

src/base/Notifier.sol

src/libraries/PositionInfoLibrary.sol

src/interfaces/IPositionManager.sol

src/PositionManager.sol

hensha256 · 2024-08-29T21:05:57Z

test/libraries/CalldataDecoder.t.sol

        assertEq(amount0Min, _amount0Min);
        assertEq(amount1Min, _amount1Min);
    }

+    // TODO: fix stack too deep here


is it fixed now?

yes i just removed that comment! thanks

saucepoint

very clean work per usual 😌

src/PositionManager.sol

test/mocks/MockCalldataDecoder.sol

src/PositionManager.sol

src/libraries/PositionInfoLibrary.sol

saucepoint · 2024-08-30T20:40:34Z

reminder: consider SB-86, hasSubscriber should revert if the token does not exist

stance: probably ugly to enforce, and adds gas. BUT should be natspec'd in hasSubscriber function

saucepoint

some CEI violations in _mint / _burn

https://github.com/spearbit-audits/review-uniswap-v4/issues/93

saucepoint · 2024-09-03T19:15:39Z

src/PositionManager.sol

+        bytes25 poolId = info.poolId();
+        // Store the poolKey if it is not already stored.
+        // On UniswapV4, the minimum tick spacing is 1, which means that if the tick spacing is 0, the pool key has not been set.
+        if (poolKeys[poolId].tickSpacing == 0) {
+            poolKeys[poolId] = poolKey;
+        }


let's do this write prior to _modifyLiquidity for CEI anyway (the hook still has access to tick range and poolKey regardless of CEI)

ah yeah that makes sense

saucepoint · 2024-09-03T19:18:34Z

src/PositionManager.sol

-        delete positionConfigs[tokenId];
+        positionInfo[tokenId] = PositionInfoLibrary.EMPTY_POSITION_INFO;
        // Burn the token.
        _burn(tokenId);


thinking this logic should run prior to _modifyLiquidity for CEI

solmate _burn might be save to leave it, but positionInfo may want to happen before?

src/libraries/PositionInfoLibrary.sol

saucepoint · 2024-09-05T20:35:18Z

test/libraries/PositionInfoLibrary.t.sol

+        info = info.setSubscribe();
+        assertEq(info.hasSubscriber(), true);
+        assertEq(info.tickLower(), tickLower);
+        assertEq(info.tickUpper(), tickUpper);
+        assertEq(info.poolId(), bytes25(PoolId.unwrap(poolKey.toId())));


library itself looks good, but as an insane person we should assert that the tickLower/tickUpper/poolId is NOT changed by .setSubscribe

i.e. asserting that the info data is the same before/after .setSubscribe

hensha256 · 2024-09-05T21:20:16Z

src/PositionManager.sol

+        // fee delta can be ignored as this is a new position
+        (BalanceDelta liquidityDelta,) =
+            _modifyLiquidity(info, poolKey, liquidity.toInt256(), bytes32(tokenId), hookData);
+        (liquidityDelta).validateMaxIn(amount0Max, amount1Max);


Suggested change

(liquidityDelta).validateMaxIn(amount0Max, amount1Max);

liquidityDelta.validateMaxIn(amount0Max, amount1Max);

hensha256 · 2024-09-05T21:31:23Z

src/PositionManager.sol

                liquidityDelta: liquidityChange,
                salt: salt
            }),
            hookData
        );

-        if (positionConfigs[uint256(salt)].hasSubscriber()) {
-            _notifyModifyLiquidity(uint256(salt), config, liquidityChange, feesAccrued);
+        // TODO: Audit issue for burn, decide if we want to keep this and also unsubscribe.


snreynolds added 2 commits August 27, 2024 20:37

messy checkpoint

c423b33

update subscribers

e497389

saucepoint reviewed Aug 28, 2024

View reviewed changes

.forge-snapshots/PositionManager_mint_native.snap Outdated Show resolved Hide resolved

hensha256 reviewed Aug 28, 2024

View reviewed changes

stack too deep, permit error

830fdf5

snreynolds requested review from hensha256 and saucepoint August 28, 2024 20:38

saucepoint reviewed Aug 29, 2024

View reviewed changes

snreynolds commented Aug 29, 2024

View reviewed changes

snreynolds added 2 commits August 29, 2024 00:44

use packed, no via-ir, but stack too deep concerning for tests

8eb66b3

use external

7e8fc42

hensha256 reviewed Aug 29, 2024

View reviewed changes

snreynolds added 2 commits August 29, 2024 12:33

review comments

f94823f

merge confs

a77fb26

hensha256 reviewed Aug 29, 2024

View reviewed changes

snreynolds added 2 commits August 29, 2024 16:13

clear lower 8 bits on unsubscribe

547c893

move _pay, minimize diff

e695f43

snreynolds changed the title ~~wip add reserve mapping for pool key, store tL, tU~~ wip add reverse mapping for pool key, store tL, tU Aug 29, 2024

snreynolds added 7 commits August 29, 2024 16:30

pr comments

df58a26

fix: test_fuzz_erc721Permit_SignatureDeadlineExpired

d6d1353

naming

7fd6b5b

fix

84cc704

natspec

ec4ebdd

fix: stack too deep in tests

5a88042

remove console

fe1d709

snreynolds changed the title ~~wip add reverse mapping for pool key, store tL, tU~~ feat: add reverse mapping for pool key, store tL, tU Aug 29, 2024

merge main

31a22de

snreynolds marked this pull request as ready for review August 29, 2024 20:50

hensha256 reviewed Aug 29, 2024

View reviewed changes

move PositionConfig to shared testing infra

ca37b8c

snreynolds requested review from saucepoint and hensha256 August 29, 2024 21:26

remove pool key checker lib

ec14729

saucepoint previously approved these changes Aug 29, 2024

View reviewed changes

src/PositionManager.sol Outdated Show resolved Hide resolved

test/mocks/MockCalldataDecoder.sol Show resolved Hide resolved

src/PositionManager.sol Outdated Show resolved Hide resolved

src/libraries/PositionInfoLibrary.sol Show resolved Hide resolved

pr commmeeennnnttsss

21ae901

snreynolds dismissed saucepoint’s stale review via 21ae901 August 29, 2024 23:33

fmt

b29c5fe

saucepoint reviewed Sep 3, 2024

View reviewed changes

0age reviewed Sep 4, 2024

View reviewed changes

src/libraries/PositionInfoLibrary.sol Show resolved Hide resolved

snreynolds added 3 commits September 5, 2024 01:08

fix: merge conflicts

ca8aea7

remove mask

2200534

comments

48f5376

saucepoint reviewed Sep 5, 2024

View reviewed changes

hensha256 reviewed Sep 5, 2024

View reviewed changes

remove comment

89957f4

hensha256 previously approved these changes Sep 5, 2024

View reviewed changes

comments

d995a11

snreynolds dismissed hensha256’s stale review via d995a11 September 5, 2024 21:48

fix conflicts

c87fd9e

hensha256 previously approved these changes Sep 5, 2024

View reviewed changes

merge conflicts

0f2bf0c

snreynolds dismissed hensha256’s stale review via 0f2bf0c September 5, 2024 22:11

add mask 200 bits

c256823

hensha256 approved these changes Sep 5, 2024

View reviewed changes

snreynolds merged commit 2b63bd7 into main Sep 5, 2024
3 checks passed

snreynolds deleted the reverse-mapping branch September 5, 2024 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add reverse mapping for pool key, store tL, tU #310

feat: add reverse mapping for pool key, store tL, tU #310

snreynolds commented Aug 28, 2024 •

edited

Loading

hensha256 commented Aug 28, 2024

snreynolds commented Aug 28, 2024

saucepoint left a comment

saucepoint Aug 29, 2024

snreynolds Aug 29, 2024

snreynolds Aug 29, 2024

hensha256 Aug 29, 2024

hensha256 Aug 29, 2024

snreynolds Aug 29, 2024

hensha256 Aug 29, 2024

snreynolds Aug 29, 2024

saucepoint left a comment •

edited

Loading

saucepoint commented Aug 30, 2024

saucepoint left a comment

saucepoint Sep 3, 2024

snreynolds Sep 5, 2024

saucepoint Sep 3, 2024

saucepoint Sep 3, 2024

saucepoint Sep 5, 2024

hensha256 Sep 5, 2024

hensha256 Sep 5, 2024

	(liquidityDelta).validateMaxIn(amount0Max, amount1Max);
	liquidityDelta.validateMaxIn(amount0Max, amount1Max);

feat: add reverse mapping for pool key, store tL, tU #310

feat: add reverse mapping for pool key, store tL, tU #310

Conversation

snreynolds commented Aug 28, 2024 • edited Loading

hensha256 commented Aug 28, 2024

snreynolds commented Aug 28, 2024

saucepoint 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

saucepoint left a comment • edited Loading

Choose a reason for hiding this comment

saucepoint commented Aug 30, 2024

saucepoint 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

snreynolds commented Aug 28, 2024 •

edited

Loading

saucepoint left a comment •

edited

Loading