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

Add v4 NonfungiblePositionManager skeleton #76

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
fe9305e
add helper contracts and interfaces for position manager
tinaszheng Nov 21, 2023
0b01a0a
run forge fmt
tinaszheng Nov 21, 2023
74ad1a2
remove custom timestamp getter
tinaszheng Nov 21, 2023
30479c6
use custom errors and IER1271.isValidSignature.selector
tinaszheng Nov 21, 2023
349ddb8
use using .. for syntax for TransferHelper
tinaszheng Nov 21, 2023
906fb65
add erics custom error handler
tinaszheng Nov 21, 2023
0152e5c
remove ChainId library
tinaszheng Nov 21, 2023
e5fc9dd
use currency library to combine sweepETH and sweepToken
tinaszheng Nov 21, 2023
54d55fd
use Currency library for pay method
tinaszheng Nov 21, 2023
a2200ac
use solmate SafeTransferLib instead
tinaszheng Nov 21, 2023
b4f84ee
remove erc721permit for now
tinaszheng Nov 21, 2023
ad5c503
revert changes to TransferHelper
tinaszheng Nov 22, 2023
04a7a1d
put using .. for in contract header
tinaszheng Nov 22, 2023
f9d830e
move custom errors inside contracts
tinaszheng Nov 22, 2023
0d87708
run format
tinaszheng Nov 22, 2023
02a5cd2
update version
tinaszheng Nov 22, 2023
d261d58
add v4 position manager contracts
tinaszheng Nov 21, 2023
7f544b1
add Currency class and fix override error
tinaszheng Nov 21, 2023
7cc8c66
remove reference to erc721enumerable from concrete
tinaszheng Nov 21, 2023
c52f05a
use custom errors
tinaszheng Nov 21, 2023
05c9543
use ERC721 for now
tinaszheng Nov 21, 2023
ca01507
move custom errors inside contract + inherit from ILockCallback
tinaszheng Nov 28, 2023
4a7d82a
use poolkey in mint params
tinaszheng Nov 28, 2023
6e82a2e
rename variables
tinaszheng Nov 28, 2023
3166045
add periphery immutable state
tinaszheng Nov 29, 2023
ad34755
update position manager skeleton with liquidity management file
tinaszheng Nov 29, 2023
aad107a
fix inheritance order
tinaszheng Nov 29, 2023
87e0b2f
move stuff around
tinaszheng Nov 29, 2023
c64e82e
move addLiquidity functions to liquiditymanagement
tinaszheng Nov 29, 2023
b72452b
update comment + delete unused file
tinaszheng Nov 29, 2023
5d8a888
remove stuff for initializing pools for now
tinaszheng Nov 29, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 201 additions & 0 deletions contracts/NonfungiblePositionManagerV4.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {IERC721Metadata} from "@openzeppelin/contracts/token/ERC721/extensions/IERC721Metadata.sol";
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
import {PoolKey} from "@uniswap/v4-core/contracts/types/PoolKey.sol";
import {PoolId, PoolIdLibrary} from "@uniswap/v4-core/contracts/types/PoolId.sol";
import {IPoolManager} from "@uniswap/v4-core/contracts/interfaces/IPoolManager.sol";
import {Currency} from "@uniswap/v4-core/contracts/types/Currency.sol";

import {INonfungiblePositionManagerV4} from "./interfaces/INonfungiblePositionManagerV4.sol";
import {PeripheryValidation} from "./base/PeripheryValidation.sol";
import {PeripheryPayments} from "./base/PeripheryPayments.sol";
import {PeripheryImmutableState} from "./base/PeripheryImmutableState.sol";
import {SelfPermit} from "./base/SelfPermit.sol";
import {LiquidityManagement} from "./base/LiquidityManagement.sol";
import {Multicall} from "./base/Multicall.sol";

contract NonfungiblePositionManagerV4 is
INonfungiblePositionManagerV4,
ERC721,
PeripheryImmutableState,
PeripheryValidation,
LiquidityManagement,
SelfPermit,
Multicall
{
using PoolIdLibrary for PoolKey;

error InvalidTokenID();
error NotApproved();
error NotCleared();
error NonexistentToken();

// details about the Uniswap position
struct TokenPosition {
// the nonce for permits
uint96 nonce;
// the address that is approved for spending this token
address operator;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// the hashed poolKey of the pool with which this token is connected
PoolId poolId;
// the tick range of the position
int24 tickLower;
int24 tickUpper;
// the liquidity of the position
uint128 liquidity;
// the fee growth of the aggregate position as of the last action on the individual position
uint256 feeGrowthInside0LastX128;
uint256 feeGrowthInside1LastX128;
// how many uncollected tokens are owed to the position, as of the last computation
uint128 tokensOwed0;
uint128 tokensOwed1;
}

/// @dev Pool keys by poolIds
mapping(bytes32 => PoolKey) private _poolIdToPoolKey;

/// @dev The token ID position data
mapping(uint256 => TokenPosition) private _positions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah good point

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// @dev The ID of the next token that will be minted. Skips 0
uint176 private _nextId = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we call this _nextTokenId? We have tokenIds and poolIds so just want to be clear when we have id var names


/// @dev The address of the token descriptor contract, which handles generating token URIs for position tokens
address private immutable _tokenDescriptor;

constructor(IPoolManager _poolManager, address _tokenDescriptor_)
PeripheryImmutableState(_poolManager)
ERC721("Uniswap V4 Positions NFT-V1", "UNI-V4-POS")
{
_tokenDescriptor = _tokenDescriptor_;
}

/// @inheritdoc INonfungiblePositionManagerV4
function positions(uint256 tokenId)
external
view
override
returns (
uint96 nonce,
address operator,
Currency currency0,
Currency currency1,
uint24 fee,
int24 tickLower,
int24 tickUpper,
uint128 liquidity,
uint256 feeGrowthInside0LastX128,
uint256 feeGrowthInside1LastX128,
uint128 tokensOwed0,
uint128 tokensOwed1
)
{
TokenPosition memory position = _positions[tokenId];
if (PoolId.unwrap(position.poolId) == 0) revert InvalidTokenID();
PoolKey memory poolKey = _poolIdToPoolKey[PoolId.unwrap(position.poolId)];
return (
position.nonce,
position.operator,
poolKey.currency0,
poolKey.currency1,
poolKey.fee,
position.tickLower,
position.tickUpper,
position.liquidity,
position.feeGrowthInside0LastX128,
position.feeGrowthInside1LastX128,
position.tokensOwed0,
position.tokensOwed1
);
}

/// @inheritdoc INonfungiblePositionManagerV4
function mint(MintParams calldata params)
external
payable
override
checkDeadline(params.deadline)
returns (uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1)
{
// TODO: implement this
}

/// @inheritdoc INonfungiblePositionManagerV4
function increaseLiquidity(IncreaseLiquidityParams calldata params)
external
payable
override
checkDeadline(params.deadline)
returns (uint128 liquidity, uint256 amount0, uint256 amount1)
{
// TODO: implement this
}

/// @inheritdoc INonfungiblePositionManagerV4
function decreaseLiquidity(DecreaseLiquidityParams calldata params)
external
payable
override
isAuthorizedForToken(params.tokenId)
checkDeadline(params.deadline)
returns (uint256 amount0, uint256 amount1)
{
// TODO: implement this
}

/// @inheritdoc INonfungiblePositionManagerV4
function collect(CollectParams calldata params)
external
payable
override
isAuthorizedForToken(params.tokenId)
returns (uint256 amount0, uint256 amount1)
{
// TODO: implement this
}

/// @inheritdoc INonfungiblePositionManagerV4
function burn(uint256 tokenId) external payable override isAuthorizedForToken(tokenId) {
TokenPosition storage position = _positions[tokenId];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the checks below will all be sloads. I think you can copy to memory, do the checks (mloads) below and then delete the storage var with delete _positions[tokenId];

if (position.liquidity != 0 || position.tokensOwed0 != 0 || position.tokensOwed1 != 0) revert NotCleared();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

delete _positions[tokenId];
_burn(tokenId);
}

modifier isAuthorizedForToken(uint256 tokenId) {
if (!_isApprovedOrOwner(msg.sender, tokenId)) revert NotApproved();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. I think we are not using the most up to date erc721 contract from in openzeppelin?

Also I prefer using solmate packages although I think for erc721 open zeppelin may have a few more helpers. @hensha256 thoughts here?

_;
}

function tokenURI(uint256 tokenId) public view override(ERC721, IERC721Metadata) returns (string memory) {
// TODO: implement this
}

/// @inheritdoc IERC721
function getApproved(uint256 tokenId) public view override(ERC721, IERC721) returns (address) {
if (!_exists(tokenId)) revert NonexistentToken();

return _positions[tokenId].operator;
}

/// @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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_positions[tokenId].operator = to;
emit Approval(ownerOf(tokenId), to, tokenId);
}

function tokenByIndex(uint256 index) external view returns (uint256) {
// TODO: implement this
}

function tokenOfOwnerByIndex(address owner, uint256 index) external view returns (uint256) {
// TODO: implement this
}

function totalSupply() external view returns (uint256) {
// TODO: implement this
}
}
103 changes: 103 additions & 0 deletions contracts/base/LiquidityManagement.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {PoolKey} from "@uniswap/v4-core/contracts/types/PoolKey.sol";
import {BalanceDelta} from "@uniswap/v4-core/contracts/types/BalanceDelta.sol";
import {ILockCallback} from "@uniswap/v4-core/contracts/interfaces/callback/ILockCallback.sol";
import {TickMath} from "@uniswap/v4-core/contracts/libraries/TickMath.sol";
import {BalanceDelta} from "@uniswap/v4-core/contracts/types/BalanceDelta.sol";
import {IPoolManager} from "@uniswap/v4-core/contracts/interfaces/IPoolManager.sol";
import {Currency, CurrencyLibrary} from "@uniswap/v4-core/contracts/types/Currency.sol";
import {PoolId, PoolIdLibrary} from "@uniswap/v4-core/contracts/types/PoolId.sol";

import {LiquidityAmounts} from "../libraries/LiquidityAmounts.sol";
import {PeripheryImmutableState} from "./PeripheryImmutableState.sol";
import {PeripheryPayments} from "./PeripheryPayments.sol";

/// @title Liquidity management functions
/// @notice Internal functions for safely managing liquidity in Uniswap V4
abstract contract LiquidityManagement is ILockCallback, PeripheryImmutableState, PeripheryPayments {
using CurrencyLibrary for Currency;
using PoolIdLibrary for PoolKey;

error PriceSlippage();

enum CallbackType {AddLiquidity}

struct CallbackData {
CallbackType callbackType;
address sender;
bytes params;
}

struct AddLiquidityParams {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

PoolKey poolKey;
int24 tickLower;
int24 tickUpper;
uint256 amount0Desired;
uint256 amount1Desired;
uint256 amount0Min;
uint256 amount1Min;
bytes hookData;
}

/// @notice Add liquidity to an initialized pool
function addLiquidity(AddLiquidityParams memory params)
internal
returns (uint128 liquidity, uint256 amount0, uint256 amount1)
{
(liquidity, amount0, amount1) = abi.decode(
poolManager.lock(abi.encode(CallbackData(CallbackType.AddLiquidity, msg.sender, abi.encode(params)))),
(uint128, uint256, uint256)
);
}

function addLiquidityCallback(AddLiquidityParams memory params)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

internal
returns (uint128 liquidity, BalanceDelta delta)
{
(uint160 sqrtPriceX96,,,) = poolManager.getSlot0(params.poolKey.toId());
uint160 sqrtRatioAX96 = TickMath.getSqrtRatioAtTick(params.tickLower);
uint160 sqrtRatioBX96 = TickMath.getSqrtRatioAtTick(params.tickUpper);
liquidity = LiquidityAmounts.getLiquidityForAmounts(
sqrtPriceX96, sqrtRatioAX96, sqrtRatioBX96, params.amount0Desired, params.amount1Desired
);
delta = poolManager.modifyPosition(
params.poolKey,
IPoolManager.ModifyPositionParams(params.tickLower, params.tickUpper, int256(int128(liquidity))),
params.hookData
);
if (
uint256(int256(delta.amount0())) < params.amount0Min || uint256(int256(delta.amount1())) < params.amount1Min
) revert PriceSlippage();
}

function settleDeltas(address from, PoolKey memory poolKey, BalanceDelta delta) internal {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if (delta.amount0() > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pay(poolKey.currency0, from, address(poolManager), uint256(int256(delta.amount0())));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are we handling native?

poolManager.settle(poolKey.currency0);
} else if (delta.amount0() < 0) {
poolManager.take(poolKey.currency0, address(this), uint128(-delta.amount0()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

AddLiquidityParams memory params = abi.decode(callbackData.params, (AddLiquidityParams));
(uint128 liquidity, BalanceDelta delta) = addLiquidityCallback(params);
settleDeltas(callbackData.sender, params.poolKey, delta);
return abi.encode(liquidity, delta.amount0(), delta.amount1());
}

// TODO: handle decrease liquidity here
return abi.encode(0);
}
}
33 changes: 33 additions & 0 deletions contracts/base/Multicall.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.19;

import {IMulticall} from "../interfaces/IMulticall.sol";

/// @title Multicall
/// @notice Enables calling multiple methods in a single call to the contract
abstract contract Multicall is IMulticall {
/// @inheritdoc IMulticall
function multicall(bytes[] calldata data) public payable override returns (bytes[] memory results) {
results = new bytes[](data.length);
for (uint256 i = 0; i < data.length; i++) {
(bool success, bytes memory result) = address(this).delegatecall(data[i]);

if (!success) {
// handle custom errors
if (result.length == 4) {
assembly {
revert(add(result, 0x20), mload(result))
}
}
Comment on lines +17 to +21

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Next 5 lines from https://ethereum.stackexchange.com/a/83577
if (result.length < 68) revert();
assembly {
result := add(result, 0x04)
}
revert(abi.decode(result, (string)));
}

results[i] = result;
}
}
}
15 changes: 15 additions & 0 deletions contracts/base/PeripheryImmutableState.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {IPoolManager} from "@uniswap/v4-core/contracts/interfaces/IPoolManager.sol";
import {IPeripheryImmutableState} from "../interfaces/IPeripheryImmutableState.sol";

/// @title Immutable state
/// @notice Immutable state used by periphery contracts
abstract contract PeripheryImmutableState is IPeripheryImmutableState {
IPoolManager public immutable override poolManager;

constructor(IPoolManager _poolManager) {
poolManager = _poolManager;
}
}
41 changes: 41 additions & 0 deletions contracts/base/PeripheryPayments.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {ERC20} from "solmate/tokens/ERC20.sol";
import {Currency, CurrencyLibrary} from "@uniswap/v4-core/contracts/types/Currency.sol";
import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";
import {IPeripheryPayments} from "../interfaces/IPeripheryPayments.sol";

abstract contract PeripheryPayments is IPeripheryPayments {
using CurrencyLibrary for Currency;
using SafeTransferLib for address;
using SafeTransferLib for ERC20;

error InsufficientToken();
error NativeTokenTransferFrom();

/// @inheritdoc IPeripheryPayments
function sweepToken(Currency currency, uint256 amountMinimum, address recipient) public payable override {
uint256 balanceCurrency = currency.balanceOfSelf();
if (balanceCurrency < amountMinimum) revert InsufficientToken();

if (balanceCurrency > 0) {
currency.transfer(recipient, balanceCurrency);
}
}

/// @param currency The currency to pay
/// @param payer The entity that must pay
/// @param recipient The entity that will receive payment
/// @param value The amount to pay
function pay(Currency currency, address payer, address recipient, uint256 value) internal {
if (payer == address(this)) {
// pay with tokens already in the contract (for the exact input multihop case)
currency.transfer(recipient, value);
} else {
if (currency.isNative()) revert NativeTokenTransferFrom();
// pull payment
ERC20(Currency.unwrap(currency)).safeTransferFrom(payer, recipient, value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
Loading