Position Manager #120

saucepoint · 2024-06-07T19:17:46Z

Related Issue

Which issue does this pull request resolve?

Description of changes

The initial position manager. The incomplete items below to be made into new issues/PRs

…on-manager

… into position-manager

* chore: update v4-core:latest (Uniswap#105) * update core * rename lockAcquired to unlockCallback * update core; temporary path hack in remappings * update v4-core; remove remapping * wip: fix compatibility * update core; fix renaming of swap fee to lp fee * update core; fix events * update core; address liquidity salt and modify liquidity return values * fix incorrect delta accounting when modifying liquidity * fix todo, use CurrencySettleTake * remove deadcode * update core; use StateLibrary; update sqrtRatio to sqrtPrice * fix beforeSwap return signatures * forge fmt; remove commented out code * update core (wow gas savings) * update core * update core * update core; hook flags LSB * update core * update core * chore: update v4 core (Uniswap#115) * Update v4-core * CurrencySettleTake -> CurrencySettler * Snapshots * compiling but very broken * replace PoolStateLibrary * update currency settle take * compiling * wip * use v4-core's forge-std * test liquidity increase * additional fixes for collection and liquidity decrease * test migration * replace old implementation with new --------- Signed-off-by: saucepoint <[email protected]> Co-authored-by: 0x57 <[email protected]>

saucepoint · 2024-06-12T14:31:15Z

.gitmodules

-[submodule "lib/forge-std"]
-	path = lib/forge-std
-	url = https://github.com/foundry-rs/forge-std


instead of maintaining two versions of forge-std, figured we can just use v4-core's forge-std. its a bit more up to update with some of the cheatcodes

makes sense

snreynolds · 2024-06-11T13:51:12Z

contracts/NonfungiblePositionManager.sol

+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.24;
+
+import {ERC721} from "openzeppelin-contracts/contracts/token/ERC721/ERC721.sol";


want to make sure we support permit on ERC721, we'll have to roll our own like we did in V3

also would say in general we're moving towards using more solmate dependencies

Copied ERC721Permit from v3. Stuck with OZ for now since they have helpful modifiers and their _beforeTokenTransfer is a bit easier to use than overriding transfer

snreynolds · 2024-06-11T13:56:33Z

contracts/NonfungiblePositionManager.sol

+    using LiquidityRangeIdLibrary for LiquidityRange;
+    using StateLibrary for IPoolManager;
+
+    /// @dev The ID of the next token that will be minted. Skips 0


is there a particular reason we skip 0? maybe v3 did something similar but I don't know why?

snreynolds · 2024-06-12T17:07:42Z

.gitmodules

-[submodule "lib/forge-std"]
-	path = lib/forge-std
-	url = https://github.com/foundry-rs/forge-std


makes sense

snreynolds · 2024-06-12T17:13:02Z

contracts/types/LiquidityRange.sol

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


suggestion to move this into IPositionManager.sol.. in core most (if not all?) of struct definitions are in respective interfaces

contracts/NonfungiblePositionManager.sol

snreynolds · 2024-06-12T17:35:14Z

contracts/base/BaseLiquidityHandler.sol

+    constructor(IPoolManager _poolManager) ImmutableState(_poolManager) {}
+
+    function _unlockCallback(bytes calldata data) internal override returns (bytes memory) {
+        (bool success, bytes memory returnData) = address(this).call(data);


how come we aren't supporting batched calls under the lock?

contracts/base/BaseLiquidityHandler.sol

snreynolds · 2024-06-12T18:01:05Z

contracts/NonfungiblePositionManager.sol

+        LiquidityRange range;
+    }
+
+    mapping(uint256 tokenId => TokenPosition position) public tokenPositions;


maybe this will become more clear to me.. but its not clear why we have this mapping and the positions mapping? seems like this just stores LiquidityRange? ... is that really necessary? can we not just pass that in?

and then we only need one mapping:
positions[msg.sender][rangeId]

a Position is keyed by its owner and the range

some interfaces (increaseLiquidity, decreaseLiquidity, and collect) operate on tokenId, so we need a way to recover Position from tokenId

snreynolds · 2024-06-12T18:33:09Z

contracts/NonfungiblePositionManager.sol

+    }
+
+    modifier isAuthorizedForToken(uint256 tokenId) {
+        require(_isApprovedOrOwner(msg.sender, tokenId), "Not approved");


hm I think _isApprovedOrOwner actually got deprecated in the most up to date version of the OZ ERC721 contracts.

solmates version also doesnt use this

so we should look into updating? or is there a specific reason to still use this?

can look into updating -- grabbed it from v3

contracts/libraries/CurrencySenderLibrary.sol

snreynolds · 2024-06-12T18:55:44Z

contracts/base/BaseLiquidityHandler.sol

+            delta = toBalanceDelta(int128(token0Owed), int128(token1Owed));
+
+            // sending tokens to the owner
+            if (token0Owed > 0) range.key.currency0.send(poolManager, owner, token0Owed, useClaims);


i personally dont think we need to support sending 6909 to user.

i do think this contract could keep them as 6909s though (& maybe just by default it should? i think it would be cheaper)

* wip: consolidation * further consolidation * consolidate to single file * yay no more stack too deep * some code comments

contracts/base/BaseLiquidityManagement.sol

contracts/libraries/CurrencySettleTake.sol

Co-authored-by: 0x57 <[email protected]>

gabririgo · 2024-06-17T07:16:15Z

contracts/interfaces/INonfungiblePositionManager.sol

maintaining the same naming as in the v3 NPM will result in selector clash for contracts/protocols that map selectors to target applications, i.e. diamond pattern. Would it be possible to rename the methods to something like "methodV4", so to allow external protocols to easily interact with v3 and v4?

UPDATE: as the input parameters are different, the resulting function hashes will be different as well. The Uni V4 input bytes calldata hookData does not exist in v3, which should guarantee that the v4 selectors be different. However, it would be nice to assert that there is no clashing possibility with v3.

its something we can keep in mind, but we expect to keep the interfaces as clean as possible

saucepoint · 2024-06-20T14:09:22Z

replaced by #124

saucepoint and others added 30 commits February 22, 2024 22:23

misc version bump; will conflict but can resolve later

1d0e566

defining types and different levels of abstractions

4f8bbd2

wip

ee27f4f

merge in main

30ac060

misc fixes with main:latest

d1c5897

basic mint

7a134a5

begin moving tests to fuzz

7bd2996

test for slippage

307f4bb

burning

109caf4

decrease liquidity

1bf080f

mint transfer burn, liquidityOf accounting

40f042c

wip

6b1c7cb

refactor to use CurrencySettleTake

fa511d0

basic fee collection

a0e0a44

wip

0d936d4

misc fix

4be3c2a

fee collection for independent same-range parties

7fa4c54

LiquidityPosition -> LiquidityRange

aae9697

erc20 fee collection

5dec534

decrease liquidity with fee collection

1196c6a

wip test decrease liquidity on same range

3d317e8

reworked fuzzers; more testing on fee claims for liquidity decreasing

31a70cb

Merge remote-tracking branch 'parent/lock-and-batch-call' into positi…

714913e

…on-manager

Merge branch 'position-manager' of github.com:saucepoint/v4-periphery…

ddf5771

… into position-manager

test fixes for flipped deltas

3c56d48

wip

f4275cc

test coverage for increase liquidity cases

245cc3e

preliminary gas benchmarks

f971b3d

fix conflicts

d86a8c2

saucepoint marked this pull request as ready for review June 12, 2024 14:18

saucepoint commented Jun 12, 2024

View reviewed changes

cleanup: TODOs and imports

52b304e

snreynolds reviewed Jun 12, 2024

View reviewed changes

saucepoint and others added 2 commits June 12, 2024 17:37

Position manager Consolidate (#3)

af67661

* wip: consolidation * further consolidation * consolidate to single file * yay no more stack too deep * some code comments

use currency settler syntax

48f38c4

qiwu7 reviewed Jun 13, 2024

View reviewed changes

contracts/base/BaseLiquidityManagement.sol Outdated Show resolved Hide resolved

qiwu7 reviewed Jun 13, 2024

View reviewed changes

contracts/libraries/CurrencySettleTake.sol Outdated Show resolved Hide resolved

saucepoint and others added 4 commits June 13, 2024 17:05

use v4-core's gas snapshot

c8ce67b

use snapLastCall and isolate for posm benchmarks

da91136

Update contracts/libraries/CurrencySettleTake.sol

18600bd

Co-authored-by: 0x57 <[email protected]>

use v4-core's solmate its more recent

f52adcf

gabririgo reviewed Jun 17, 2024

View reviewed changes

saucepoint added 3 commits June 17, 2024 09:44

use v4-core's openzeppelin-contracts

07cc628

add ERC721Permit

240c8e1

feedback: memory hookData

1cb1948

saucepoint closed this Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Position Manager #120

Position Manager #120

saucepoint commented Jun 7, 2024 •

edited

Loading

saucepoint Jun 12, 2024 •

edited

Loading

snreynolds Jun 12, 2024

snreynolds Jun 11, 2024

saucepoint Jun 17, 2024

snreynolds Jun 11, 2024

snreynolds Jun 12, 2024

snreynolds Jun 12, 2024

snreynolds Jun 12, 2024

snreynolds Jun 12, 2024

saucepoint Jun 12, 2024

snreynolds Jun 12, 2024

saucepoint Jun 12, 2024

snreynolds Jun 12, 2024

gabririgo Jun 17, 2024 •

edited

Loading

saucepoint Jun 20, 2024

saucepoint commented Jun 20, 2024


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

		struct LiquidityRange {

Position Manager #120

Position Manager #120

Conversation

saucepoint commented Jun 7, 2024 • edited Loading

Related Issue

Description of changes

saucepoint Jun 12, 2024 • edited Loading

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

gabririgo Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saucepoint commented Jun 20, 2024

saucepoint commented Jun 7, 2024 •

edited

Loading

saucepoint Jun 12, 2024 •

edited

Loading

gabririgo Jun 17, 2024 •

edited

Loading