Add v4 NonfungiblePositionManager skeleton #76

tinaszheng · 2023-11-21T03:05:12Z

Description of changes

This PR adds the base skeleton for NonfungiblePositionManagerV4

So this is mostly copied from v3-periphery but I made a few changes:

Pool IDs are now just PoolKeys (instead of having a separate counter => address mapping)
I removed references to WETH9 specifically (until we decide on what to do about WETH in v4)
Added LiquidityManagement to manage lock/lock callback logic
Added createAndInitializePoolIfNecessary directly to the contract and func args take in PoolKey instead of ids
Added tokenByIndex, tokenOfOwnerByIndex, and totalSupply - think these were added to ERC721Enumerable?

TODO (next steps):

fill in function definitions (everywhere that says // TODO: implement this)
add tests
Use permit2 version of ERC721Permit?

tinaszheng · 2023-11-29T17:38:11Z

contracts/base/LiquidityManagement.sol

+import {PeripheryImmutableState} from "./PeripheryImmutableState.sol";
+
+abstract contract LiquidityManagement is ILockCallback, ILiquidityManagement, PeripheryImmutableState {
+    function mintEntry(MintParams memory params)


just an example of how LiquidityManagement can be used with contracts/NonfungiblePositionManagerV4.sol - mintEntry calls the lock, then lockAcquired handles rest of the codepath within this file

zhongeric · 2023-12-01T15:59:54Z

contracts/base/Multicall.sol

+                if (result.length == 4) {
+                    assembly {
+                        revert(add(result, 0x20), mload(result))
+                    }
+                }


oops mark pointed out that custom errors can revert with params so this doesn't necessarily catch all custom errors 😅 , we could probably just revert with the reason again or we can look for a fix

hensha256 · 2023-12-04T18:20:32Z

contracts/NonfungiblePositionManagerV4.sol

+    mapping(bytes32 => PoolKey) private _poolIdToPoolKey;
+
+    /// @dev The token ID position data
+    mapping(uint256 => TokenPosition) private _positions;


is there a reason for this to be private? im down for this to be public i can imagine other contracts or interfaces wanting to look up position info

thinking they probably want to use the positions getter for getting position data, since it includes helpers for unwrapping poolId - otherwise they're kind of exposed to our internal representation of how we store pools right? they'll need both _poolIdToPoolKey and _positions to get useful position data

oh yeah good point

nit: Would just add to the natspec that they key is specifically the tokenId (tokenId) ie mapping from tokenId to position data

hensha256 · 2023-12-04T18:22:45Z

contracts/NonfungiblePositionManagerV4.sol

+    /// @inheritdoc INonfungiblePositionManagerV4
+    function burn(uint256 tokenId) external payable override isAuthorizedForToken(tokenId) {
+        TokenPosition storage position = _positions[tokenId];
+        if (position.liquidity != 0 || position.tokensOwed0 != 0 || position.tokensOwed1 != 0) revert NotCleared();


do we need to make sure theyve collected all their rewards before they can burn? or does this already check that?

ok ive dug into it and these checks should be enough for burn

hensha256 · 2023-12-04T18:25:35Z

contracts/base/PeripheryPayments.sol

+        } else {
+            if (currency.isNative()) revert NativeTokenTransferFrom();
+            // pull payment
+            ERC20(Currency.unwrap(currency)).safeTransferFrom(payer, recipient, value);


do we want to use Permit2 for this? or are we good with approve and transferFrom?

hensha256 · 2023-12-04T18:28:17Z

contracts/interfaces/IPeripheryPayments.sol

+/// @title Periphery Payments
+/// @notice Functions to ease deposits and withdrawals of ETH
+interface IPeripheryPayments {
+    // TODO: figure out if we still need unwrapWETH9 from v3?


hmmm good question. I think its more of an opinionated decision nowadays... I would lean towards not adding it? People can add liquidity in ETH and remove it in ETH, people can add liquidity in WETH and remove it in WETH. It feels opinionated now to allow the WETH people to remove it as ETH if we arent doing it the other way around?

however we do like to be helpful if we think its a common usecase of course

yeah agreed on not making it opinionated

hensha256 · 2023-12-04T19:01:51Z

contracts/base/LiquidityManagement.sol

+            pay(poolKey.currency0, from, address(poolManager), uint256(int256(delta.amount0())));
+            poolManager.settle(poolKey.currency0);
+        } else if (delta.amount0() < 0) {
+            poolManager.take(poolKey.currency0, address(this), uint128(-delta.amount0()));


i feel like we might want this to send directly to the recipient instead of to address(this) - so that we dont then have to send it onwards which is extra gas?

snreynolds · 2023-12-04T20:51:34Z

contracts/interfaces/INonfungiblePositionManagerV4.sol

+    }
+
+    /// @notice Creates a new position wrapped in a NFT
+    /// @dev Call this when the pool does exist and is initialized. Note that if the pool is created but not initialized


what is a pool that is created but not initialized?

snreynolds · 2023-12-04T20:52:40Z

contracts/interfaces/INonfungiblePositionManagerV4.sol

+        payable
+        returns (uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1);
+
+    struct IncreaseLiquidityParams {


I prefer all structs to be defined at the top/grouped together. I think that is just a consistent styling choice I've noticed for most of our contracts

snreynolds · 2023-12-04T21:10:28Z

contracts/base/LiquidityManagement.sol

+        bytes params;
+    }
+
+    struct AddLiquidityParams {


It seems weird to have bothAddLiquidityParams and IncreaseLiquidityParams. Like hard to know why you use one and not the other.

Also, its possible that we actually can just have ModifyLiquidityParams that handle both increase and decrease..

And we should be consistent with the naming add/remove vs. increase/decrease. I prefer increase/decrease I think

snreynolds · 2023-12-04T21:13:11Z

contracts/base/LiquidityManagement.sol

+
+    function lockAcquired(bytes calldata data) external override returns (bytes memory) {
+        CallbackData memory callbackData = abi.decode(data, (CallbackData));
+        if (callbackData.callbackType == CallbackType.AddLiquidity) {


I'm guessing this will be a big block of if statements?

probably will then want to move the settle logic and the return statement outside the if statement since we will want to settle/return deltas in all of the cases

snreynolds · 2023-12-04T21:14:36Z

contracts/base/LiquidityManagement.sol

+        );
+    }
+
+    function addLiquidityCallback(AddLiquidityParams memory params)


addLiquidityCallback is a strange name for an internal handler function because callbacks imply an external caller. Also for internal functions we can use the _

maybe _addLiquidity?

snreynolds · 2023-12-04T21:41:29Z

contracts/NonfungiblePositionManagerV4.sol

+        // the nonce for permits
+        uint96 nonce;
+        // the address that is approved for spending this token
+        address operator;


hm this is kind of strange because you can have more than 1 operator with the 721 spec... so not sure why we save just one. did we do the same in v3? if so.. why?

snreynolds · 2023-12-04T21:43:57Z

contracts/NonfungiblePositionManagerV4.sol

+    }
+
+    /// @dev Overrides _approve to use the operator in the position, which is packed with the position permit nonce
+    function _approve(address to, uint256 tokenId) internal override(ERC721) {


hmmm I'm kinda confused with the approval/auth scheme we are defining. It sounds like we are trying to override the traditional 721 spec but we are still using(not dis-allowing) the old 721 interfaces.

and we are using the old traditional 721 checks like _isApprovedOrOwner which doesnt look at the operator in the TokenPositions struct

Also it seems like we want to allow permits... but where is all that sig verification?

snreynolds · 2023-12-04T21:45:39Z

contracts/base/LiquidityManagement.sol

+    }
+
+    function settleDeltas(address from, PoolKey memory poolKey, BalanceDelta delta) internal {
+        if (delta.amount0() > 0) {


note that this won't work for pools that have custom accounting

snreynolds · 2023-12-04T21:45:47Z

contracts/base/LiquidityManagement.sol

+
+    function settleDeltas(address from, PoolKey memory poolKey, BalanceDelta delta) internal {
+        if (delta.amount0() > 0) {
+            pay(poolKey.currency0, from, address(poolManager), uint256(int256(delta.amount0())));


how are we handling native?

snreynolds · 2023-12-04T21:47:59Z

contracts/base/LiquidityManagement.sol

+        ) revert PriceSlippage();
+    }
+
+    function settleDeltas(address from, PoolKey memory poolKey, BalanceDelta delta) internal {


One high level thing here... I don't think we want to use take and settle. We will want to use mint/ burn.

When the user is withdrawing fully we will use take/settle though to give them the underlying erc20s. But when we are in the state where the position manager is still managing positions for a user we just want to hold claims to underlying imo (which will interface with the 6909 path)

snreynolds · 2024-07-09T16:55:35Z

Closing in favor of #141

tinaszheng added 2 commits November 20, 2023 21:37

add helper contracts and interfaces for position manager

fe9305e

run forge fmt

0b01a0a

tinaszheng changed the title ~~add v4 position manager contracts~~ Add v4 NonfungiblePositionManager Nov 21, 2023

tinaszheng requested review from snreynolds, hensha256 and marktoda November 21, 2023 03:06

tinaszheng changed the title ~~Add v4 NonfungiblePositionManager~~ Add v4 NonfungiblePositionManager (WIP) Nov 21, 2023

tinaszheng mentioned this pull request Nov 21, 2023

Add base contracts and interfaces used in PositionManager #75

Merged

tinaszheng added 4 commits November 21, 2023 14:34

remove custom timestamp getter

74ad1a2

use custom errors and IER1271.isValidSignature.selector

30479c6

use using .. for syntax for TransferHelper

349ddb8

add erics custom error handler

906fb65

tinaszheng force-pushed the tina/position-manager branch from 2834b62 to 38e78db Compare November 21, 2023 21:54

tinaszheng added 5 commits November 21, 2023 17:35

remove ChainId library

0152e5c

use currency library to combine sweepETH and sweepToken

e5fc9dd

use Currency library for pay method

54d55fd

use solmate SafeTransferLib instead

a2200ac

remove erc721permit for now

b4f84ee

tinaszheng force-pushed the tina/position-manager branch from 88119a9 to a26a6e9 Compare November 21, 2023 23:10

tinaszheng added 10 commits November 22, 2023 12:39

revert changes to TransferHelper

ad5c503

put using .. for in contract header

04a7a1d

move custom errors inside contracts

f9d830e

run format

0d87708

update version

02a5cd2

add v4 position manager contracts

d261d58

add Currency class and fix override error

7f544b1

remove reference to erc721enumerable from concrete

7cc8c66

use custom errors

c52f05a

use ERC721 for now

05c9543

tinaszheng force-pushed the tina/position-manager branch from a26a6e9 to 05c9543 Compare November 26, 2023 22:43

tinaszheng added 3 commits November 27, 2023 19:54

move custom errors inside contract + inherit from ILockCallback

ca01507

use poolkey in mint params

4a7d82a

rename variables

6e82a2e

tinaszheng force-pushed the tina/add-base-contracts-for-pm branch from 02a5cd2 to 324976e Compare November 28, 2023 02:33

Base automatically changed from tina/add-base-contracts-for-pm to main November 28, 2023 16:41

tinaszheng added 2 commits November 29, 2023 11:54

add periphery immutable state

3166045

update position manager skeleton with liquidity management file

ad34755

tinaszheng force-pushed the tina/position-manager branch from ca4e05c to ad34755 Compare November 29, 2023 17:14

tinaszheng changed the title ~~Add v4 NonfungiblePositionManager (WIP)~~ Add v4 NonfungiblePositionManager skeleton Nov 29, 2023

fix inheritance order

aad107a

tinaszheng commented Nov 29, 2023

View reviewed changes

tinaszheng added 4 commits November 29, 2023 15:17

move stuff around

87e0b2f

move addLiquidity functions to liquiditymanagement

c64e82e

update comment + delete unused file

b72452b

remove stuff for initializing pools for now

5d8a888

zhongeric reviewed Dec 1, 2023

View reviewed changes

hensha256 reviewed Dec 4, 2023

View reviewed changes

snreynolds requested changes Dec 4, 2023

View reviewed changes

hensha256 added the posm position manager label Jun 26, 2024

snreynolds closed this Jul 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add v4 NonfungiblePositionManager skeleton #76

Add v4 NonfungiblePositionManager skeleton #76

tinaszheng commented Nov 21, 2023 •

edited

Loading

tinaszheng Nov 29, 2023

zhongeric Dec 1, 2023

hensha256 Dec 4, 2023

tinaszheng Dec 4, 2023

hensha256 Dec 4, 2023

snreynolds Dec 4, 2023

hensha256 Dec 4, 2023

hensha256 Dec 4, 2023

hensha256 Dec 4, 2023

hensha256 Dec 4, 2023

hensha256 Dec 4, 2023

tinaszheng Dec 4, 2023

hensha256 Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds Dec 4, 2023

snreynolds commented Jul 9, 2024

Add v4 NonfungiblePositionManager skeleton #76

Add v4 NonfungiblePositionManager skeleton #76

Conversation

tinaszheng commented Nov 21, 2023 • edited Loading

Description of changes

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

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

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 Jul 9, 2024

tinaszheng commented Nov 21, 2023 •

edited

Loading