From 8ce4ed6f250ee8a64fd2a15c2f2fc49a705f7278 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Fri, 28 Jun 2024 11:23:13 -0400 Subject: [PATCH 01/14] checkpointing --- .../autocompound_exactUnclaimedFees.snap | 2 +- ...exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .../autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .../increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- contracts/NonfungiblePositionManager.sol | 8 ++++++-- contracts/base/BaseLiquidityManagement.sol | 5 +++++ ...ntDemo.sol => TransientLiquidityDelta.sol} | 19 +++++++++++++------ 9 files changed, 30 insertions(+), 14 deletions(-) rename contracts/libraries/{TransientDemo.sol => TransientLiquidityDelta.sol} (73%) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 2e536b9f..66840744 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -262501 \ No newline at end of file +264058 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index 553b43e9..e109a0c7 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -194874 \ No newline at end of file +196431 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index ac41e55d..24fdaa94 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -283040 \ No newline at end of file +284597 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index 6691256e..8fedeb73 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -175258 \ No newline at end of file +176815 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index babfbeeb..79c05ace 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -150847 \ No newline at end of file +152404 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index 33617d43..618059cd 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -472846 \ No newline at end of file +475856 \ No newline at end of file diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index 4f4056f1..f3b1940d 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -18,6 +18,7 @@ import {TickMath} from "@uniswap/v4-core/src/libraries/TickMath.sol"; import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol"; import {TransientStateLibrary} from "@uniswap/v4-core/src/libraries/TransientStateLibrary.sol"; import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; +import {TransientLiquidityDelta} from "./libraries/TransientLiquidityDelta.sol"; contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidityManagement, ERC721Permit { using CurrencyLibrary for Currency; @@ -27,6 +28,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit using StateLibrary for IPoolManager; using TransientStateLibrary for IPoolManager; using SafeCast for uint256; + using TransientLiquidityDelta for address; /// @dev The ID of the next token that will be minted. Skips 0 uint256 private _nextId = 1; @@ -69,10 +71,12 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit ) public payable returns (uint256 tokenId, BalanceDelta delta) { // TODO: optimization, read/write manager.isUnlocked to avoid repeated external calls for batched execution if (manager.isUnlocked()) { - BalanceDelta thisDelta; - (delta, thisDelta) = _increaseLiquidity(recipient, range, liquidity, hookData); + _increaseLiquidity(recipient, range, liquidity, hookData); // TODO: should be triggered by zeroOut in _execute... + delta = recipient.getBalanceDelta(range.poolKey.currency0, range.poolKey.currency1); + BalanceDelta thisDelta = address(this).getBalanceDelta(range.poolKey.currency0, range.poolKey.currency1); + _closeCallerDeltas(delta, range.poolKey.currency0, range.poolKey.currency1, recipient, false); _closeThisDeltas(thisDelta, range.poolKey.currency0, range.poolKey.currency1); diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index 63d325de..0c4f4471 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -24,6 +24,7 @@ import {IBaseLiquidityManagement} from "../interfaces/IBaseLiquidityManagement.s import {PositionLibrary} from "../libraries/Position.sol"; import {BalanceDeltaExtensionLibrary} from "../libraries/BalanceDeltaExtensionLibrary.sol"; import {LiquidityDeltaAccounting} from "../libraries/LiquidityDeltaAccounting.sol"; +import {TransientLiquidityDelta} from "../libraries/TransientLiquidityDelta.sol"; import "forge-std/console2.sol"; @@ -40,6 +41,8 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb using PositionLibrary for IBaseLiquidityManagement.Position; using BalanceDeltaExtensionLibrary for BalanceDelta; using LiquidityDeltaAccounting for BalanceDelta; + using TransientLiquidityDelta for BalanceDelta; + using TransientLiquidityDelta for Currency; mapping(address owner => mapping(LiquidityRangeId rangeId => Position)) public positions; @@ -101,6 +104,8 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb // Calculate the portion of the liquidityDelta that is attributable to the caller. // We must account for fees that might be owed to other users on the same range. (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); + callerDelta.flush(owner, range.poolKey.currency0, range.poolKey.currency1); + thisDelta.flush(address(this), range.poolKey.currency0, range.poolKey.currency1); // Update position storage, flushing the callerDelta value to tokensOwed first if necessary. // If callerDelta > 0, then even after investing callerFeesAccrued, the caller still has some amount to collect that were not added into the position so they are accounted to tokensOwed and removed from the final callerDelta returned. diff --git a/contracts/libraries/TransientDemo.sol b/contracts/libraries/TransientLiquidityDelta.sol similarity index 73% rename from contracts/libraries/TransientDemo.sol rename to contracts/libraries/TransientLiquidityDelta.sol index 2845878d..8f473927 100644 --- a/contracts/libraries/TransientDemo.sol +++ b/contracts/libraries/TransientLiquidityDelta.sol @@ -1,12 +1,13 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity ^0.8.24; -import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; +import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; /// @title a library to store callers' currency deltas in transient storage /// @dev this library implements the equivalent of a mapping, as transient storage can only be accessed in assembly library TransientLiquidityDelta { + /// @notice calculates which storage slot a delta should be stored in for a given caller and currency function _computeSlot(address caller_, Currency currency) internal pure returns (bytes32 hashSlot) { assembly { @@ -18,8 +19,8 @@ library TransientLiquidityDelta { /// @notice Flush a BalanceDelta into transient storage for a given holder function flush(BalanceDelta delta, address holder, Currency currency0, Currency currency1) internal { - setDelta(currency0, holder, delta.amount0()); - setDelta(currency1, holder, delta.amount1()); + addDelta(currency0, holder, delta.amount0()); + addDelta(currency1, holder, delta.amount1()); } function addDelta(Currency currency, address caller, int128 delta) internal { @@ -31,7 +32,7 @@ library TransientLiquidityDelta { } } - function subDelta(Currency currency, address caller, int128 delta) internal { + function subtractDelta(Currency currency, address caller, int128 delta) internal { bytes32 hashSlot = _computeSlot(caller, currency); assembly { let oldValue := tload(hashSlot) @@ -40,8 +41,13 @@ library TransientLiquidityDelta { } } + function getBalanceDelta(address holder, Currency currency0, Currency currency1) internal view returns (BalanceDelta delta) { + delta = toBalanceDelta(getDelta(currency0, holder), getDelta(currency1, holder)); + } + + /// Copied from v4-core/src/libraries/CurrencyDelta.sol: /// @notice sets a new currency delta for a given caller and currency - function setDelta(Currency currency, address caller, int256 delta) internal { + function setDelta(Currency currency, address caller, int128 delta) internal { bytes32 hashSlot = _computeSlot(caller, currency); assembly { @@ -50,7 +56,8 @@ library TransientLiquidityDelta { } /// @notice gets a new currency delta for a given caller and currency - function getDelta(Currency currency, address caller) internal view returns (int256 delta) { + // TODO: is returning 128 bits safe? + function getDelta(Currency currency, address caller) internal view returns (int128 delta) { bytes32 hashSlot = _computeSlot(caller, currency); assembly { From 5f7e9c17d7ee4b5bcae77881f181446fe82d2770 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Fri, 28 Jun 2024 11:47:00 -0400 Subject: [PATCH 02/14] move decrease and collect to transient storage --- .../autocompound_exactUnclaimedFees.snap | 2 +- ...exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .../autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc20.snap | 2 +- .../decreaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .../increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- contracts/NonfungiblePositionManager.sol | 22 ++++++++++++++----- contracts/base/BaseLiquidityManagement.sol | 16 +++++++++++--- .../libraries/TransientLiquidityDelta.sol | 12 ++++++++-- 11 files changed, 48 insertions(+), 18 deletions(-) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 66840744..7c212457 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -264058 \ No newline at end of file +265947 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index e109a0c7..1da11488 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -196431 \ No newline at end of file +198320 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index 24fdaa94..a33617b1 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -284597 \ No newline at end of file +286486 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index d780cfa9..8b851a43 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1 +1 @@ -180891 \ No newline at end of file +184337 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index d968e558..39ae6b81 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1 +1 @@ -180903 \ No newline at end of file +184349 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index 8fedeb73..a8a02e1d 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -176815 \ No newline at end of file +178704 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index 79c05ace..b1ca1097 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -152404 \ No newline at end of file +154293 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index 618059cd..2e1b54ab 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -475856 \ No newline at end of file +476807 \ No newline at end of file diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index f3b1940d..2a06533a 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -119,8 +119,11 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit TokenPosition memory tokenPos = tokenPositions[tokenId]; if (manager.isUnlocked()) { - BalanceDelta thisDelta; - (delta, thisDelta) = _increaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); + _increaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); + + delta = tokenPos.owner.getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); + BalanceDelta thisDelta = + address(this).getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); // TODO: should be triggered by zeroOut in _execute... _closeCallerDeltas( @@ -143,7 +146,12 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit TokenPosition memory tokenPos = tokenPositions[tokenId]; if (manager.isUnlocked()) { - (delta, thisDelta) = _decreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); + _decreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); + + // TODO: move to zeroOut() in unlockAndExecute() + delta = tokenPos.owner.getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); + thisDelta = + address(this).getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); _closeCallerDeltas( delta, tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1, tokenPos.owner, claims ); @@ -187,8 +195,12 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit { TokenPosition memory tokenPos = tokenPositions[tokenId]; if (manager.isUnlocked()) { - BalanceDelta thisDelta; - (delta, thisDelta) = _collect(tokenPos.owner, tokenPos.range, hookData); + _collect(tokenPos.owner, tokenPos.range, hookData); + + // TODO: move to zeroOut() in unlockAndExecute() + delta = tokenPos.owner.getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); + BalanceDelta thisDelta = + address(this).getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); _closeCallerDeltas( delta, tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1, tokenPos.owner, claims ); diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index 0c4f4471..ca9e21a9 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -43,6 +43,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb using LiquidityDeltaAccounting for BalanceDelta; using TransientLiquidityDelta for BalanceDelta; using TransientLiquidityDelta for Currency; + using TransientLiquidityDelta for address; mapping(address owner => mapping(LiquidityRangeId rangeId => Position)) public positions; @@ -65,6 +66,8 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb if (callerDelta1 < 0) currency1.settle(manager, owner, uint256(int256(-callerDelta1)), claims); else if (callerDelta1 > 0) currency1.take(manager, owner, uint128(callerDelta1), claims); + + owner.close(currency0, currency1); } function _modifyLiquidity(address owner, LiquidityRange memory range, int256 liquidityChange, bytes memory hookData) @@ -104,8 +107,6 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb // Calculate the portion of the liquidityDelta that is attributable to the caller. // We must account for fees that might be owed to other users on the same range. (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); - callerDelta.flush(owner, range.poolKey.currency0, range.poolKey.currency1); - thisDelta.flush(address(this), range.poolKey.currency0, range.poolKey.currency1); // Update position storage, flushing the callerDelta value to tokensOwed first if necessary. // If callerDelta > 0, then even after investing callerFeesAccrued, the caller still has some amount to collect that were not added into the position so they are accounted to tokensOwed and removed from the final callerDelta returned. @@ -119,6 +120,8 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb (tokensOwed, callerDelta, thisDelta) = _moveCallerDeltaToTokensOwed(false, tokensOwed, callerDelta, thisDelta); } + callerDelta.flush(owner, range.poolKey.currency0, range.poolKey.currency1); + thisDelta.flush(address(this), range.poolKey.currency0, range.poolKey.currency1); position.addTokensOwed(tokensOwed); position.addLiquidity(liquidityToAdd); @@ -136,6 +139,8 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb // Burn the receipt for tokens owed to this address. if (delta0 < 0) currency0.settle(manager, address(this), uint256(int256(-delta0)), true); if (delta1 < 0) currency1.settle(manager, address(this), uint256(int256(-delta1)), true); + + address(this).close(currency0, currency1); } function _moveCallerDeltaToTokensOwed( @@ -157,7 +162,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb return (tokensOwed, callerDelta, thisDelta); } - + /// Any outstanding amounts owed to the caller from the underlying modify call must be collected explicitly with `collect`. function _decreaseLiquidity( address owner, @@ -189,6 +194,8 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb (tokensOwed, callerDelta, thisDelta) = _moveCallerDeltaToTokensOwed(false, tokensOwed, callerDelta, thisDelta); } + callerDelta.flush(owner, range.poolKey.currency0, range.poolKey.currency1); + thisDelta.flush(address(this), range.poolKey.currency0, range.poolKey.currency1); position.addTokensOwed(tokensOwed); position.subtractLiquidity(liquidityToRemove); @@ -222,6 +229,9 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb callerDelta = callerDelta + tokensOwed; thisDelta = thisDelta - tokensOwed; + callerDelta.flush(owner, range.poolKey.currency0, range.poolKey.currency1); + thisDelta.flush(address(this), range.poolKey.currency0, range.poolKey.currency1); + position.clearTokensOwed(); } diff --git a/contracts/libraries/TransientLiquidityDelta.sol b/contracts/libraries/TransientLiquidityDelta.sol index 8f473927..2f3259cf 100644 --- a/contracts/libraries/TransientLiquidityDelta.sol +++ b/contracts/libraries/TransientLiquidityDelta.sol @@ -7,7 +7,6 @@ import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; /// @title a library to store callers' currency deltas in transient storage /// @dev this library implements the equivalent of a mapping, as transient storage can only be accessed in assembly library TransientLiquidityDelta { - /// @notice calculates which storage slot a delta should be stored in for a given caller and currency function _computeSlot(address caller_, Currency currency) internal pure returns (bytes32 hashSlot) { assembly { @@ -17,6 +16,11 @@ library TransientLiquidityDelta { } } + function close(address holder, Currency currency0, Currency currency1) internal { + setDelta(currency0, holder, 0); + setDelta(currency1, holder, 0); + } + /// @notice Flush a BalanceDelta into transient storage for a given holder function flush(BalanceDelta delta, address holder, Currency currency0, Currency currency1) internal { addDelta(currency0, holder, delta.amount0()); @@ -41,7 +45,11 @@ library TransientLiquidityDelta { } } - function getBalanceDelta(address holder, Currency currency0, Currency currency1) internal view returns (BalanceDelta delta) { + function getBalanceDelta(address holder, Currency currency0, Currency currency1) + internal + view + returns (BalanceDelta delta) + { delta = toBalanceDelta(getDelta(currency0, holder), getDelta(currency1, holder)); } From 5d780c8fadcfd6ab77ea4ca1457c0bfa97ba0c4e Mon Sep 17 00:00:00 2001 From: saucepoint Date: Fri, 28 Jun 2024 11:56:19 -0400 Subject: [PATCH 03/14] remove returns since they are now saved to transient storage --- .forge-snapshots/autocompound_exactUnclaimedFees.snap | 2 +- ...ompound_exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .forge-snapshots/autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc20.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .forge-snapshots/increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- contracts/base/BaseLiquidityManagement.sol | 11 ++++++----- 9 files changed, 14 insertions(+), 13 deletions(-) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 7c212457..97da5cff 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -265947 \ No newline at end of file +265935 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index 1da11488..9dc064aa 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -198320 \ No newline at end of file +198308 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index a33617b1..f0221ade 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -286486 \ No newline at end of file +286474 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index 8b851a43..4649c8ac 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1 +1 @@ -184337 \ No newline at end of file +184325 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index 39ae6b81..8b851a43 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1 +1 @@ -184349 \ No newline at end of file +184337 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index a8a02e1d..58a71668 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -178704 \ No newline at end of file +178692 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index b1ca1097..a3560dfc 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -154293 \ No newline at end of file +154281 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index 2e1b54ab..44d60635 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -476807 \ No newline at end of file +476792 \ No newline at end of file diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index ca9e21a9..3cfdf210 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -93,7 +93,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb LiquidityRange memory range, uint256 liquidityToAdd, bytes memory hookData - ) internal returns (BalanceDelta callerDelta, BalanceDelta thisDelta) { + ) internal { // Note that the liquidityDelta includes totalFeesAccrued. The totalFeesAccrued is returned separately for accounting purposes. (BalanceDelta liquidityDelta, BalanceDelta totalFeesAccrued) = _modifyLiquidity(owner, range, liquidityToAdd.toInt256(), hookData); @@ -106,7 +106,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb // Calculate the portion of the liquidityDelta that is attributable to the caller. // We must account for fees that might be owed to other users on the same range. - (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); + (BalanceDelta callerDelta, BalanceDelta thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); // Update position storage, flushing the callerDelta value to tokensOwed first if necessary. // If callerDelta > 0, then even after investing callerFeesAccrued, the caller still has some amount to collect that were not added into the position so they are accounted to tokensOwed and removed from the final callerDelta returned. @@ -169,7 +169,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb LiquidityRange memory range, uint256 liquidityToRemove, bytes memory hookData - ) internal returns (BalanceDelta callerDelta, BalanceDelta thisDelta) { + ) internal { (BalanceDelta liquidityDelta, BalanceDelta totalFeesAccrued) = _modifyLiquidity(owner, range, -(liquidityToRemove.toInt256()), hookData); @@ -180,7 +180,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb (BalanceDelta callerFeesAccrued) = _updateFeeGrowth(range, position); // Account for fees accrued to other users on the same range. - (callerDelta, thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); + (BalanceDelta callerDelta, BalanceDelta thisDelta) = liquidityDelta.split(callerFeesAccrued, totalFeesAccrued); BalanceDelta tokensOwed; @@ -203,8 +203,9 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb function _collect(address owner, LiquidityRange memory range, bytes memory hookData) internal - returns (BalanceDelta callerDelta, BalanceDelta thisDelta) { + BalanceDelta callerDelta; + BalanceDelta thisDelta; Position storage position = positions[owner][range.toId()]; // Only call modify if there is still liquidty in this position. From c08bef160f2408619badec8a0117265fb6ab7a5e Mon Sep 17 00:00:00 2001 From: saucepoint Date: Fri, 28 Jun 2024 19:19:01 -0400 Subject: [PATCH 04/14] draft: delta closing --- contracts/NonfungiblePositionManager.sol | 52 ++++--------------- contracts/base/BaseLiquidityManagement.sol | 4 +- .../INonfungiblePositionManager.sol | 3 +- .../libraries/TransientLiquidityDelta.sol | 26 ++++++++++ 4 files changed, 39 insertions(+), 46 deletions(-) diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index 2a06533a..ef49fc96 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -28,7 +28,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit using StateLibrary for IPoolManager; using TransientStateLibrary for IPoolManager; using SafeCast for uint256; - using TransientLiquidityDelta for address; + using TransientLiquidityDelta for Currency; /// @dev The ID of the next token that will be minted. Skips 0 uint256 private _nextId = 1; @@ -41,12 +41,12 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit ERC721Permit("Uniswap V4 Positions NFT-V1", "UNI-V4-POS", "1") {} - function unlockAndExecute(bytes[] memory data) public returns (bytes memory) { - return manager.unlock(abi.encode(data)); + function unlockAndExecute(bytes[] memory data, Currency[] memory currencies) public returns (bytes memory) { + return manager.unlock(abi.encode(data, currencies)); } function _unlockCallback(bytes calldata payload) internal override returns (bytes memory) { - bytes[] memory data = abi.decode(payload, (bytes[])); + (bytes[] memory data, Currency[] memory currencies) = abi.decode(payload, (bytes[], Currency[])); bool success; bytes memory returnData; @@ -55,9 +55,12 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit (success, returnData) = address(this).call(data[i]); if (!success) revert("EXECUTE_FAILED"); } - // zeroOut(); - return returnData; + // close the deltas + for (uint256 i; i < currencies.length; i++) { + currencies[i].close(manager, msg.sender); + currencies[i].close(manager, address(this)); + } } // NOTE: more gas efficient as LiquidityAmounts is used offchain @@ -73,13 +76,6 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit if (manager.isUnlocked()) { _increaseLiquidity(recipient, range, liquidity, hookData); - // TODO: should be triggered by zeroOut in _execute... - delta = recipient.getBalanceDelta(range.poolKey.currency0, range.poolKey.currency1); - BalanceDelta thisDelta = address(this).getBalanceDelta(range.poolKey.currency0, range.poolKey.currency1); - - _closeCallerDeltas(delta, range.poolKey.currency0, range.poolKey.currency1, recipient, false); - _closeThisDeltas(thisDelta, range.poolKey.currency0, range.poolKey.currency1); - // mint receipt token _mint(recipient, (tokenId = _nextId++)); tokenPositions[tokenId] = TokenPosition({owner: recipient, range: range}); @@ -120,16 +116,6 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit if (manager.isUnlocked()) { _increaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); - - delta = tokenPos.owner.getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); - BalanceDelta thisDelta = - address(this).getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); - - // TODO: should be triggered by zeroOut in _execute... - _closeCallerDeltas( - delta, tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1, tokenPos.owner, claims - ); - _closeThisDeltas(thisDelta, tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); } else { bytes[] memory data = new bytes[](1); data[0] = abi.encodeWithSelector(this.increaseLiquidity.selector, tokenId, liquidity, hookData, claims); @@ -147,15 +133,6 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit if (manager.isUnlocked()) { _decreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); - - // TODO: move to zeroOut() in unlockAndExecute() - delta = tokenPos.owner.getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); - thisDelta = - address(this).getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); - _closeCallerDeltas( - delta, tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1, tokenPos.owner, claims - ); - _closeThisDeltas(thisDelta, tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); } else { bytes[] memory data = new bytes[](1); data[0] = abi.encodeWithSelector(this.decreaseLiquidity.selector, tokenId, liquidity, hookData, claims); @@ -195,16 +172,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit { TokenPosition memory tokenPos = tokenPositions[tokenId]; if (manager.isUnlocked()) { - _collect(tokenPos.owner, tokenPos.range, hookData); - - // TODO: move to zeroOut() in unlockAndExecute() - delta = tokenPos.owner.getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); - BalanceDelta thisDelta = - address(this).getBalanceDelta(tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); - _closeCallerDeltas( - delta, tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1, tokenPos.owner, claims - ); - _closeThisDeltas(thisDelta, tokenPos.range.poolKey.currency0, tokenPos.range.poolKey.currency1); + _collect(recipient, tokenPos.range, hookData); } else { bytes[] memory data = new bytes[](1); data[0] = abi.encodeWithSelector(this.collect.selector, tokenId, recipient, hookData, claims); diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index 3cfdf210..662c54a0 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -201,9 +201,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb position.subtractLiquidity(liquidityToRemove); } - function _collect(address owner, LiquidityRange memory range, bytes memory hookData) - internal - { + function _collect(address owner, LiquidityRange memory range, bytes memory hookData) internal { BalanceDelta callerDelta; BalanceDelta thisDelta; Position storage position = positions[owner][range.toId()]; diff --git a/contracts/interfaces/INonfungiblePositionManager.sol b/contracts/interfaces/INonfungiblePositionManager.sol index 6b029fd0..88d63ae0 100644 --- a/contracts/interfaces/INonfungiblePositionManager.sol +++ b/contracts/interfaces/INonfungiblePositionManager.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.24; import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; +import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; import {LiquidityRange} from "../types/LiquidityRange.sol"; interface INonfungiblePositionManager { @@ -68,7 +69,7 @@ interface INonfungiblePositionManager { /// @notice Execute a batch of external calls by unlocking the PoolManager /// @param data an array of abi.encodeWithSelector(, ) for each call /// @return delta The final delta changes of the caller - function unlockAndExecute(bytes[] memory data) external returns (bytes memory); + function unlockAndExecute(bytes[] memory data, Currency[] memory currencies) external returns (bytes memory); /// @notice Returns the fees owed for a position. Includes unclaimed fees + custodied fees + claimable fees /// @param tokenId The ID of the position diff --git a/contracts/libraries/TransientLiquidityDelta.sol b/contracts/libraries/TransientLiquidityDelta.sol index 2f3259cf..ef9926af 100644 --- a/contracts/libraries/TransientLiquidityDelta.sol +++ b/contracts/libraries/TransientLiquidityDelta.sol @@ -1,12 +1,17 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity ^0.8.24; +import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; +import {CurrencySettleTake} from "../libraries/CurrencySettleTake.sol"; + /// @title a library to store callers' currency deltas in transient storage /// @dev this library implements the equivalent of a mapping, as transient storage can only be accessed in assembly library TransientLiquidityDelta { + using CurrencySettleTake for Currency; + /// @notice calculates which storage slot a delta should be stored in for a given caller and currency function _computeSlot(address caller_, Currency currency) internal pure returns (bytes32 hashSlot) { assembly { @@ -45,6 +50,27 @@ library TransientLiquidityDelta { } } + function close(Currency currency, IPoolManager manager, address holder) internal { + // getDelta(currency, holder); + bytes32 hashSlot = _computeSlot(caller, currency); + int128 delta; + assembly { + delta := tload(hashSlot) + } + + // close the delta by paying or taking + if (delta < 0) { + currency.settle(manager, holder, uint256(-delta), false); + } else { + currency.take(manager, holder, uint256(delta), false); + } + + // setDelta(0); + assembly { + tstore(hashSlot, 0) + } + } + function getBalanceDelta(address holder, Currency currency0, Currency currency1) internal view From a2186d779bc0812e968768fbbda1bb8f51878ed3 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 30 Jun 2024 00:22:22 -0400 Subject: [PATCH 05/14] wip --- .../autocompound_exactUnclaimedFees.snap | 6 +-- ...exactUnclaimedFees_exactCustodiedFees.snap | 6 +-- .../autocompound_excessFeesCredit.snap | 6 +-- .forge-snapshots/decreaseLiquidity_erc20.snap | 6 +-- .../decreaseLiquidity_erc6909.snap | 6 +-- .forge-snapshots/increaseLiquidity_erc20.snap | 6 +-- .../increaseLiquidity_erc6909.snap | 6 +-- .forge-snapshots/mintWithLiquidity.snap | 6 +-- contracts/NonfungiblePositionManager.sol | 38 ++++++++++++++++--- .../libraries/TransientLiquidityDelta.sol | 4 +- test/position-managers/Execute.t.sol | 15 ++++++-- 11 files changed, 55 insertions(+), 50 deletions(-) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 37b86092..c931974c 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1,5 +1 @@ -<<<<<<< HEAD -265935 -======= -262456 ->>>>>>> sauce/posm-batch-execute +299933 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index d3523cfe..aeba6588 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1,5 +1 @@ -<<<<<<< HEAD -198308 -======= -194829 ->>>>>>> sauce/posm-batch-execute +239395 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index 1e6fb253..4c0c834b 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1,5 +1 @@ -<<<<<<< HEAD -286474 -======= -282995 ->>>>>>> sauce/posm-batch-execute +320472 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index 141b00b4..7b956e12 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1,5 +1 @@ -<<<<<<< HEAD -184325 -======= -180479 ->>>>>>> sauce/posm-batch-execute +215156 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index ab4499cf..f251c34d 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1,5 +1 @@ -<<<<<<< HEAD -184337 -======= -180491 ->>>>>>> sauce/posm-batch-execute +215168 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index 2c35a837..ae25726e 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1,5 +1 @@ -<<<<<<< HEAD -178692 -======= -175213 ->>>>>>> sauce/posm-batch-execute +194836 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index 64062a9b..218e7c7a 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1,5 +1 @@ -<<<<<<< HEAD -154281 -======= -150802 ->>>>>>> sauce/posm-batch-execute +194848 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index 0e661026..185c902a 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1,5 +1 @@ -<<<<<<< HEAD -476792 -======= -472424 ->>>>>>> sauce/posm-batch-execute +511680 \ No newline at end of file diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index ea8d9ab4..5a9ec848 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -36,6 +36,9 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit // maps the ERC721 tokenId to the keys that uniquely identify a liquidity position (owner, range) mapping(uint256 tokenId => TokenPosition position) public tokenPositions; + // TODO: TSTORE this jawn + address internal msgSender; + constructor(IPoolManager _manager) BaseLiquidityManagement(_manager) ERC721Permit("Uniswap V4 Positions NFT-V1", "UNI-V4-POS", "1") @@ -58,9 +61,14 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit // close the deltas for (uint256 i; i < currencies.length; i++) { - currencies[i].close(manager, msg.sender); + currencies[i].close(manager, msgSender); currencies[i].close(manager, address(this)); } + + // TODO: @sara handle the return + // vanilla: return int128[2] + // batch: return int128[data.length] + return returnData; } // NOTE: more gas efficient as LiquidityAmounts is used offchain @@ -81,9 +89,14 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit _mint(recipient, (tokenId = nextTokenId++)); tokenPositions[tokenId] = TokenPosition({owner: recipient, range: range}); } else { + msgSender = msg.sender; bytes[] memory data = new bytes[](1); data[0] = abi.encodeWithSelector(this.mint.selector, range, liquidity, deadline, recipient, hookData); - bytes memory result = unlockAndExecute(data); + + Currency[] memory currencies = new Currency[](2); + currencies[0] = range.poolKey.currency0; + currencies[1] = range.poolKey.currency1; + bytes memory result = unlockAndExecute(data, currencies); delta = abi.decode(result, (BalanceDelta)); } } @@ -118,9 +131,14 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit if (manager.isUnlocked()) { _increaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); } else { + msgSender = msg.sender; bytes[] memory data = new bytes[](1); data[0] = abi.encodeWithSelector(this.increaseLiquidity.selector, tokenId, liquidity, hookData, claims); - bytes memory result = unlockAndExecute(data); + + Currency[] memory currencies = new Currency[](2); + currencies[0] = tokenPos.range.poolKey.currency0; + currencies[1] = tokenPos.range.poolKey.currency1; + bytes memory result = unlockAndExecute(data, currencies); delta = abi.decode(result, (BalanceDelta)); } } @@ -135,9 +153,14 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit if (manager.isUnlocked()) { _decreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); } else { + msgSender = msg.sender; bytes[] memory data = new bytes[](1); data[0] = abi.encodeWithSelector(this.decreaseLiquidity.selector, tokenId, liquidity, hookData, claims); - bytes memory result = unlockAndExecute(data); + + Currency[] memory currencies = new Currency[](2); + currencies[0] = tokenPos.range.poolKey.currency0; + currencies[1] = tokenPos.range.poolKey.currency1; + bytes memory result = unlockAndExecute(data, currencies); delta = abi.decode(result, (BalanceDelta)); } } @@ -175,9 +198,14 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit if (manager.isUnlocked()) { _collect(recipient, tokenPos.range, hookData); } else { + msgSender = msg.sender; bytes[] memory data = new bytes[](1); data[0] = abi.encodeWithSelector(this.collect.selector, tokenId, recipient, hookData, claims); - bytes memory result = unlockAndExecute(data); + + Currency[] memory currencies = new Currency[](2); + currencies[0] = tokenPos.range.poolKey.currency0; + currencies[1] = tokenPos.range.poolKey.currency1; + bytes memory result = unlockAndExecute(data, currencies); delta = abi.decode(result, (BalanceDelta)); } } diff --git a/contracts/libraries/TransientLiquidityDelta.sol b/contracts/libraries/TransientLiquidityDelta.sol index ef9926af..86f85c85 100644 --- a/contracts/libraries/TransientLiquidityDelta.sol +++ b/contracts/libraries/TransientLiquidityDelta.sol @@ -52,8 +52,8 @@ library TransientLiquidityDelta { function close(Currency currency, IPoolManager manager, address holder) internal { // getDelta(currency, holder); - bytes32 hashSlot = _computeSlot(caller, currency); - int128 delta; + bytes32 hashSlot = _computeSlot(holder, currency); + int256 delta; assembly { delta := tload(hashSlot) } diff --git a/test/position-managers/Execute.t.sol b/test/position-managers/Execute.t.sol index 1c1144d8..5d6b7d39 100644 --- a/test/position-managers/Execute.t.sol +++ b/test/position-managers/Execute.t.sol @@ -87,7 +87,10 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { INonfungiblePositionManager.increaseLiquidity.selector, tokenId, liquidityToAdd, ZERO_BYTES, false ); - lpm.unlockAndExecute(data); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + lpm.unlockAndExecute(data, currencies); (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, initialLiquidity + liquidityToAdd); @@ -112,7 +115,10 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { INonfungiblePositionManager.increaseLiquidity.selector, tokenId, liquidityToAdd2, ZERO_BYTES, false ); - lpm.unlockAndExecute(data); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + lpm.unlockAndExecute(data, currencies); (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, initialiLiquidity + liquidityToAdd + liquidityToAdd2); @@ -137,7 +143,10 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { INonfungiblePositionManager.increaseLiquidity.selector, tokenId, liquidityToAdd, ZERO_BYTES, false ); - lpm.unlockAndExecute(data); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + lpm.unlockAndExecute(data, currencies); (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, intialLiquidity + liquidityToAdd); From 0f394200191754bbf704a50a53418662ad5dd6da Mon Sep 17 00:00:00 2001 From: Sara Reynolds <30504811+snreynolds@users.noreply.github.com> Date: Sun, 30 Jun 2024 20:12:14 -0400 Subject: [PATCH 06/14] Sra/edits (#137) * consolidate using owner, update burn * fix: accrue deltas to caller in increase --- .../autocompound_exactUnclaimedFees.snap | 2 +- ...exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .../autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc20.snap | 2 +- .../decreaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .../increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- contracts/NonfungiblePositionManager.sol | 54 +++++++++---------- contracts/base/BaseLiquidityManagement.sol | 15 +++++- .../interfaces/IBaseLiquidityManagement.sol | 3 ++ .../INonfungiblePositionManager.sol | 10 ++-- .../libraries/TransientLiquidityDelta.sol | 6 ++- .../NonfungiblePositionManager.t.sol | 5 +- 14 files changed, 62 insertions(+), 47 deletions(-) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index c931974c..8dd96b22 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -299933 \ No newline at end of file +300126 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index aeba6588..f9df8100 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -239395 \ No newline at end of file +239588 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index 4c0c834b..eed2bed5 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -320472 \ No newline at end of file +320665 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index 7b956e12..5e4cd19e 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1 +1 @@ -215156 \ No newline at end of file +215187 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index f251c34d..e21ae2c4 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1 +1 @@ -215168 \ No newline at end of file +215199 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index ae25726e..37a69b54 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -194836 \ No newline at end of file +195029 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index 218e7c7a..23ce9120 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -194848 \ No newline at end of file +195041 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index 185c902a..39d02c26 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -511680 \ No newline at end of file +512049 \ No newline at end of file diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index 5a9ec848..594e4997 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -20,6 +20,8 @@ import {TransientStateLibrary} from "@uniswap/v4-core/src/libraries/TransientSta import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; import {TransientLiquidityDelta} from "./libraries/TransientLiquidityDelta.sol"; +import "forge-std/console2.sol"; + contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidityManagement, ERC721Permit { using CurrencyLibrary for Currency; using CurrencySettleTake for Currency; @@ -39,12 +41,18 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit // TODO: TSTORE this jawn address internal msgSender; + // TODO: Context is inherited through ERC721 and will be not useful to use _msgSender() which will be address(this) with our current mutlicall. + function _msgSenderInternal() internal override returns (address) { + return msgSender; + } + constructor(IPoolManager _manager) BaseLiquidityManagement(_manager) ERC721Permit("Uniswap V4 Positions NFT-V1", "UNI-V4-POS", "1") {} function unlockAndExecute(bytes[] memory data, Currency[] memory currencies) public returns (bytes memory) { + msgSender = msg.sender; return manager.unlock(abi.encode(data, currencies)); } @@ -65,6 +73,9 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit currencies[i].close(manager, address(this)); } + // Should just be returning the netted amount that was settled on behalf of the caller (msgSender) + // And any recipient deltas settled earlier. + // TODO: @sara handle the return // vanilla: return int128[2] // batch: return int128[data.length] @@ -77,21 +88,21 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit LiquidityRange calldata range, uint256 liquidity, uint256 deadline, - address recipient, + address owner, bytes calldata hookData ) public payable returns (BalanceDelta delta) { // TODO: optimization, read/write manager.isUnlocked to avoid repeated external calls for batched execution if (manager.isUnlocked()) { - _increaseLiquidity(recipient, range, liquidity, hookData); + _increaseLiquidity(owner, range, liquidity, hookData); // mint receipt token uint256 tokenId; - _mint(recipient, (tokenId = nextTokenId++)); - tokenPositions[tokenId] = TokenPosition({owner: recipient, range: range}); + _mint(owner, (tokenId = nextTokenId++)); + tokenPositions[tokenId] = TokenPosition({owner: owner, range: range}); } else { msgSender = msg.sender; bytes[] memory data = new bytes[](1); - data[0] = abi.encodeWithSelector(this.mint.selector, range, liquidity, deadline, recipient, hookData); + data[0] = abi.encodeWithSelector(this.mint.selector, range, liquidity, deadline, owner, hookData); Currency[] memory currencies = new Currency[](2); currencies[0] = range.poolKey.currency0; @@ -131,7 +142,6 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit if (manager.isUnlocked()) { _increaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); } else { - msgSender = msg.sender; bytes[] memory data = new bytes[](1); data[0] = abi.encodeWithSelector(this.increaseLiquidity.selector, tokenId, liquidity, hookData, claims); @@ -153,10 +163,9 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit if (manager.isUnlocked()) { _decreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); } else { - msgSender = msg.sender; bytes[] memory data = new bytes[](1); data[0] = abi.encodeWithSelector(this.decreaseLiquidity.selector, tokenId, liquidity, hookData, claims); - + Currency[] memory currencies = new Currency[](2); currencies[0] = tokenPos.range.poolKey.currency0; currencies[1] = tokenPos.range.poolKey.currency1; @@ -165,27 +174,15 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit } } - function burn(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) - external - isAuthorizedForToken(tokenId) - returns (BalanceDelta delta) - { - // TODO: Burn currently decreases and collects. However its done under different locks. - // Replace once we have the execute multicall. - // remove liquidity - TokenPosition storage tokenPosition = tokenPositions[tokenId]; - LiquidityRangeId rangeId = tokenPosition.range.toId(); - Position storage position = positions[msg.sender][rangeId]; - if (position.liquidity > 0) { - delta = decreaseLiquidity(tokenId, position.liquidity, hookData, claims); - } - - collect(tokenId, recipient, hookData, claims); - require(position.tokensOwed0 == 0 && position.tokensOwed1 == 0, "NOT_EMPTY"); - delete positions[msg.sender][rangeId]; + // TODO return type? + function burn(uint256 tokenId) public isAuthorizedForToken(tokenId) returns (BalanceDelta delta) { + // TODO: Burn currently requires a decrease and collect call before the token can be deleted. Possible to combine. + // We do not need to enforce the pool manager to be unlocked bc this function is purely clearing storage for the minted tokenId. + TokenPosition memory tokenPos = tokenPositions[tokenId]; + // Checks that the full position's liquidity has been removed and all tokens have been collected from tokensOwed. + _validateBurn(tokenPos.owner, tokenPos.range); delete tokenPositions[tokenId]; - - // burn the token + // Burn the token. _burn(tokenId); } @@ -198,7 +195,6 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit if (manager.isUnlocked()) { _collect(recipient, tokenPos.range, hookData); } else { - msgSender = msg.sender; bytes[] memory data = new bytes[](1); data[0] = abi.encodeWithSelector(this.collect.selector, tokenId, recipient, hookData, claims); diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index 662c54a0..c1c1308b 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -49,6 +49,8 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb constructor(IPoolManager _manager) ImmutableState(_manager) {} + function _msgSenderInternal() internal virtual returns (address); + function _closeCallerDeltas( BalanceDelta callerDeltas, Currency currency0, @@ -120,7 +122,9 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb (tokensOwed, callerDelta, thisDelta) = _moveCallerDeltaToTokensOwed(false, tokensOwed, callerDelta, thisDelta); } - callerDelta.flush(owner, range.poolKey.currency0, range.poolKey.currency1); + + // Accrue all deltas to the caller. + callerDelta.flush(_msgSenderInternal(), range.poolKey.currency0, range.poolKey.currency1); thisDelta.flush(address(this), range.poolKey.currency0, range.poolKey.currency1); position.addTokensOwed(tokensOwed); @@ -201,6 +205,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb position.subtractLiquidity(liquidityToRemove); } + // The recipient may not be the original owner. function _collect(address owner, LiquidityRange memory range, bytes memory hookData) internal { BalanceDelta callerDelta; BalanceDelta thisDelta; @@ -234,6 +239,14 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb position.clearTokensOwed(); } + function _validateBurn(address owner, LiquidityRange memory range) internal { + LiquidityRangeId rangeId = range.toId(); + Position storage position = positions[owner][rangeId]; + if (position.liquidity > 0) revert PositionMustBeEmpty(); + if (position.tokensOwed0 != 0 && position.tokensOwed1 != 0) revert TokensMustBeCollected(); + delete positions[owner][rangeId]; + } + function _updateFeeGrowth(LiquidityRange memory range, Position storage position) internal returns (BalanceDelta callerFeesAccrued) diff --git a/contracts/interfaces/IBaseLiquidityManagement.sol b/contracts/interfaces/IBaseLiquidityManagement.sol index b5c07dd8..6bcb6e5b 100644 --- a/contracts/interfaces/IBaseLiquidityManagement.sol +++ b/contracts/interfaces/IBaseLiquidityManagement.sol @@ -6,6 +6,9 @@ import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; import {LiquidityRange, LiquidityRangeId} from "../types/LiquidityRange.sol"; interface IBaseLiquidityManagement { + error PositionMustBeEmpty(); + error TokensMustBeCollected(); + // details about the liquidity position struct Position { // the nonce for permits diff --git a/contracts/interfaces/INonfungiblePositionManager.sol b/contracts/interfaces/INonfungiblePositionManager.sol index 32c12756..dc7fdb71 100644 --- a/contracts/interfaces/INonfungiblePositionManager.sol +++ b/contracts/interfaces/INonfungiblePositionManager.sol @@ -43,16 +43,12 @@ interface INonfungiblePositionManager { external returns (BalanceDelta delta); + // TODO Can decide if we want burn to auto encode a decrease/collect. /// @notice Burn a position and delete the tokenId - /// @dev It removes liquidity and collects fees if the position is not empty + /// @dev It enforces that there is no open liquidity or tokens to be collected /// @param tokenId The ID of the position - /// @param recipient The address to send the collected tokens to - /// @param hookData Arbitrary data passed to the hook - /// @param claims Whether the removed liquidity is sent as ERC-6909 claim tokens /// @return delta Corresponding balance changes as a result of burning the position - function burn(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) - external - returns (BalanceDelta delta); + function burn(uint256 tokenId) external returns (BalanceDelta delta); // TODO: in v3, we can partially collect fees, but what was the usecase here? /// @notice Collect fees for a position diff --git a/contracts/libraries/TransientLiquidityDelta.sol b/contracts/libraries/TransientLiquidityDelta.sol index 86f85c85..9dca43a5 100644 --- a/contracts/libraries/TransientLiquidityDelta.sol +++ b/contracts/libraries/TransientLiquidityDelta.sol @@ -6,11 +6,15 @@ import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDe import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; import {CurrencySettleTake} from "../libraries/CurrencySettleTake.sol"; +import {TransientStateLibrary} from "@uniswap/v4-core/src/libraries/TransientStateLibrary.sol"; + +import "forge-std/console2.sol"; /// @title a library to store callers' currency deltas in transient storage /// @dev this library implements the equivalent of a mapping, as transient storage can only be accessed in assembly library TransientLiquidityDelta { using CurrencySettleTake for Currency; + using TransientStateLibrary for IPoolManager; /// @notice calculates which storage slot a delta should be stored in for a given caller and currency function _computeSlot(address caller_, Currency currency) internal pure returns (bytes32 hashSlot) { @@ -58,7 +62,7 @@ library TransientLiquidityDelta { delta := tload(hashSlot) } - // close the delta by paying or taking + // TODO support claims field if (delta < 0) { currency.settle(manager, holder, uint256(-delta), false); } else { diff --git a/test/position-managers/NonfungiblePositionManager.t.sol b/test/position-managers/NonfungiblePositionManager.t.sol index 5330b731..4b8a7791 100644 --- a/test/position-managers/NonfungiblePositionManager.t.sol +++ b/test/position-managers/NonfungiblePositionManager.t.sol @@ -215,7 +215,10 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi // burn liquidity uint256 balance0BeforeBurn = currency0.balanceOfSelf(); uint256 balance1BeforeBurn = currency1.balanceOfSelf(); - BalanceDelta delta = lpm.burn(tokenId, address(this), ZERO_BYTES, false); + // TODO, encode this under one call + lpm.decreaseLiquidity(tokenId, liquidity, ZERO_BYTES, false); + lpm.collect(tokenId, address(this), ZERO_BYTES, false); + BalanceDelta delta = lpm.burn(tokenId); (,, liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, 0); From ae001d06eae968743d0d21e350126e4018c707ba Mon Sep 17 00:00:00 2001 From: saucepoint <98790946+saucepoint@users.noreply.github.com> Date: Mon, 1 Jul 2024 11:56:26 -0400 Subject: [PATCH 07/14] Rip Out Vanilla (#138) * rip out vanilla and benchmark * fix gas benchmark * check posm is the locker before allowing access to external functions * restore execute tests * posm takes as 6909; remove legacy deadcode * restore tests * move helpers to the same file --- .../autocompound_exactUnclaimedFees.snap | 2 +- ...exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .../autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc20.snap | 2 +- .../decreaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .../increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- contracts/NonfungiblePositionManager.sol | 108 ++++--------- contracts/base/BaseLiquidityManagement.sol | 45 +----- .../INonfungiblePositionManager.sol | 21 +-- .../libraries/TransientLiquidityDelta.sol | 28 ++-- test/position-managers/Execute.t.sol | 10 +- test/position-managers/FeeCollection.t.sol | 147 +++++++++--------- test/position-managers/Gas.t.sol | 105 ++++++++++--- .../position-managers/IncreaseLiquidity.t.sol | 95 ++++++----- .../NonfungiblePositionManager.t.sol | 30 ++-- test/shared/LiquidityOperations.sol | 72 +++++++++ test/shared/fuzz/LiquidityFuzzers.sol | 23 ++- 19 files changed, 390 insertions(+), 310 deletions(-) create mode 100644 test/shared/LiquidityOperations.sol diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 8dd96b22..30ef564b 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -300126 \ No newline at end of file +293482 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index f9df8100..8715f5c7 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -239588 \ No newline at end of file +225841 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index eed2bed5..87b78db1 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -320665 \ No newline at end of file +314021 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index 5e4cd19e..42c0f57b 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1 +1 @@ -215187 \ No newline at end of file +208541 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index e21ae2c4..786e11f0 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1 +1 @@ -215199 \ No newline at end of file +208553 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index 37a69b54..f872a578 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -195029 \ No newline at end of file +194298 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index 23ce9120..d9d4f89f 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -195041 \ No newline at end of file +194310 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index 39d02c26..39b5a098 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -512049 \ No newline at end of file +513250 \ No newline at end of file diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index 594e4997..e6be1e49 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -38,8 +38,9 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit // maps the ERC721 tokenId to the keys that uniquely identify a liquidity position (owner, range) mapping(uint256 tokenId => TokenPosition position) public tokenPositions; - // TODO: TSTORE this jawn + // TODO: TSTORE these jawns address internal msgSender; + bool internal unlockedByThis; // TODO: Context is inherited through ERC721 and will be not useful to use _msgSender() which will be address(this) with our current mutlicall. function _msgSenderInternal() internal override returns (address) { @@ -51,35 +52,34 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit ERC721Permit("Uniswap V4 Positions NFT-V1", "UNI-V4-POS", "1") {} - function unlockAndExecute(bytes[] memory data, Currency[] memory currencies) public returns (bytes memory) { + function unlockAndExecute(bytes[] memory data, Currency[] memory currencies) public returns (int128[] memory) { msgSender = msg.sender; - return manager.unlock(abi.encode(data, currencies)); + unlockedByThis = true; + return abi.decode(manager.unlock(abi.encode(data, currencies)), (int128[])); } function _unlockCallback(bytes calldata payload) internal override returns (bytes memory) { (bytes[] memory data, Currency[] memory currencies) = abi.decode(payload, (bytes[], Currency[])); bool success; - bytes memory returnData; + for (uint256 i; i < data.length; i++) { // TODO: bubble up the return - (success, returnData) = address(this).call(data[i]); + (success,) = address(this).call(data[i]); if (!success) revert("EXECUTE_FAILED"); } // close the deltas + int128[] memory returnData = new int128[](currencies.length); for (uint256 i; i < currencies.length; i++) { - currencies[i].close(manager, msgSender); - currencies[i].close(manager, address(this)); + returnData[i] = currencies[i].close(manager, msgSender, false); // TODO: support claims + currencies[i].close(manager, address(this), true); // position manager always takes 6909 } // Should just be returning the netted amount that was settled on behalf of the caller (msgSender) - // And any recipient deltas settled earlier. - - // TODO: @sara handle the return - // vanilla: return int128[2] - // batch: return int128[data.length] - return returnData; + // TODO: any recipient deltas settled earlier. + // @comment sauce: i dont think we can return recipient deltas since we cant parse the payload + return abi.encode(returnData); } // NOTE: more gas efficient as LiquidityAmounts is used offchain @@ -90,26 +90,13 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit uint256 deadline, address owner, bytes calldata hookData - ) public payable returns (BalanceDelta delta) { - // TODO: optimization, read/write manager.isUnlocked to avoid repeated external calls for batched execution - if (manager.isUnlocked()) { - _increaseLiquidity(owner, range, liquidity, hookData); - - // mint receipt token - uint256 tokenId; - _mint(owner, (tokenId = nextTokenId++)); - tokenPositions[tokenId] = TokenPosition({owner: owner, range: range}); - } else { - msgSender = msg.sender; - bytes[] memory data = new bytes[](1); - data[0] = abi.encodeWithSelector(this.mint.selector, range, liquidity, deadline, owner, hookData); - - Currency[] memory currencies = new Currency[](2); - currencies[0] = range.poolKey.currency0; - currencies[1] = range.poolKey.currency1; - bytes memory result = unlockAndExecute(data, currencies); - delta = abi.decode(result, (BalanceDelta)); - } + ) external payable onlyIfUnlocked { + _increaseLiquidity(owner, range, liquidity, hookData); + + // mint receipt token + uint256 tokenId; + _mint(owner, (tokenId = nextTokenId++)); + tokenPositions[tokenId] = TokenPosition({owner: owner, range: range}); } // NOTE: more expensive since LiquidityAmounts is used onchain @@ -135,43 +122,21 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit function increaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims) external isAuthorizedForToken(tokenId) - returns (BalanceDelta delta) + onlyIfUnlocked { TokenPosition memory tokenPos = tokenPositions[tokenId]; - if (manager.isUnlocked()) { - _increaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); - } else { - bytes[] memory data = new bytes[](1); - data[0] = abi.encodeWithSelector(this.increaseLiquidity.selector, tokenId, liquidity, hookData, claims); - - Currency[] memory currencies = new Currency[](2); - currencies[0] = tokenPos.range.poolKey.currency0; - currencies[1] = tokenPos.range.poolKey.currency1; - bytes memory result = unlockAndExecute(data, currencies); - delta = abi.decode(result, (BalanceDelta)); - } + _increaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); } function decreaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims) - public + external isAuthorizedForToken(tokenId) - returns (BalanceDelta delta) + onlyIfUnlocked { TokenPosition memory tokenPos = tokenPositions[tokenId]; - if (manager.isUnlocked()) { - _decreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); - } else { - bytes[] memory data = new bytes[](1); - data[0] = abi.encodeWithSelector(this.decreaseLiquidity.selector, tokenId, liquidity, hookData, claims); - - Currency[] memory currencies = new Currency[](2); - currencies[0] = tokenPos.range.poolKey.currency0; - currencies[1] = tokenPos.range.poolKey.currency1; - bytes memory result = unlockAndExecute(data, currencies); - delta = abi.decode(result, (BalanceDelta)); - } + _decreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); } // TODO return type? @@ -188,22 +153,12 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit // TODO: in v3, we can partially collect fees, but what was the usecase here? function collect(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) - public - returns (BalanceDelta delta) + external + onlyIfUnlocked { TokenPosition memory tokenPos = tokenPositions[tokenId]; - if (manager.isUnlocked()) { - _collect(recipient, tokenPos.range, hookData); - } else { - bytes[] memory data = new bytes[](1); - data[0] = abi.encodeWithSelector(this.collect.selector, tokenId, recipient, hookData, claims); - - Currency[] memory currencies = new Currency[](2); - currencies[0] = tokenPos.range.poolKey.currency0; - currencies[1] = tokenPos.range.poolKey.currency1; - bytes memory result = unlockAndExecute(data, currencies); - delta = abi.decode(result, (BalanceDelta)); - } + + _collect(recipient, tokenPos.owner, tokenPos.range, hookData); } function feesOwed(uint256 tokenId) external view returns (uint256 token0Owed, uint256 token1Owed) { @@ -234,4 +189,9 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit require(msg.sender == address(this) || _isApprovedOrOwner(msg.sender, tokenId), "Not approved"); _; } + + modifier onlyIfUnlocked() { + if (!unlockedByThis) revert MustBeUnlockedByThisContract(); + _; + } } diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index c1c1308b..79862ded 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -51,27 +51,6 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb function _msgSenderInternal() internal virtual returns (address); - function _closeCallerDeltas( - BalanceDelta callerDeltas, - Currency currency0, - Currency currency1, - address owner, - bool claims - ) internal { - int128 callerDelta0 = callerDeltas.amount0(); - int128 callerDelta1 = callerDeltas.amount1(); - // On liquidity increase, the deltas should never be > 0. - // We always 0 out a caller positive delta because it is instead accounted for in position.tokensOwed. - - if (callerDelta0 < 0) currency0.settle(manager, owner, uint256(int256(-callerDelta0)), claims); - else if (callerDelta0 > 0) currency0.take(manager, owner, uint128(callerDelta0), claims); - - if (callerDelta1 < 0) currency1.settle(manager, owner, uint256(int256(-callerDelta1)), claims); - else if (callerDelta1 > 0) currency1.take(manager, owner, uint128(callerDelta1), claims); - - owner.close(currency0, currency1); - } - function _modifyLiquidity(address owner, LiquidityRange memory range, int256 liquidityChange, bytes memory hookData) internal returns (BalanceDelta liquidityDelta, BalanceDelta totalFeesAccrued) @@ -131,22 +110,6 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb position.addLiquidity(liquidityToAdd); } - // When chaining many actions, this should be called at the very end to close out any open deltas owed to or by this contract for other users on the same range. - // This is safe because any amounts the caller should not pay or take have already been accounted for in closeCallerDeltas. - function _closeThisDeltas(BalanceDelta delta, Currency currency0, Currency currency1) internal { - int128 delta0 = delta.amount0(); - int128 delta1 = delta.amount1(); - - // Mint a receipt for the tokens owed to this address. - if (delta0 > 0) currency0.take(manager, address(this), uint128(delta0), true); - if (delta1 > 0) currency1.take(manager, address(this), uint128(delta1), true); - // Burn the receipt for tokens owed to this address. - if (delta0 < 0) currency0.settle(manager, address(this), uint256(int256(-delta0)), true); - if (delta1 < 0) currency1.settle(manager, address(this), uint256(int256(-delta1)), true); - - address(this).close(currency0, currency1); - } - function _moveCallerDeltaToTokensOwed( bool useAmount0, BalanceDelta tokensOwed, @@ -206,7 +169,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb } // The recipient may not be the original owner. - function _collect(address owner, LiquidityRange memory range, bytes memory hookData) internal { + function _collect(address recipient, address owner, LiquidityRange memory range, bytes memory hookData) internal { BalanceDelta callerDelta; BalanceDelta thisDelta; Position storage position = positions[owner][range.toId()]; @@ -233,7 +196,11 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb callerDelta = callerDelta + tokensOwed; thisDelta = thisDelta - tokensOwed; - callerDelta.flush(owner, range.poolKey.currency0, range.poolKey.currency1); + if (recipient == _msgSenderInternal()) { + callerDelta.flush(recipient, range.poolKey.currency0, range.poolKey.currency1); + } else { + callerDelta.closeDelta(manager, recipient, range.poolKey.currency0, range.poolKey.currency1, false); // TODO: allow recipient to receive claims, and add test! + } thisDelta.flush(address(this), range.poolKey.currency0, range.poolKey.currency1); position.clearTokensOwed(); diff --git a/contracts/interfaces/INonfungiblePositionManager.sol b/contracts/interfaces/INonfungiblePositionManager.sol index dc7fdb71..94cf9206 100644 --- a/contracts/interfaces/INonfungiblePositionManager.sol +++ b/contracts/interfaces/INonfungiblePositionManager.sol @@ -11,6 +11,8 @@ interface INonfungiblePositionManager { LiquidityRange range; } + error MustBeUnlockedByThisContract(); + // NOTE: more gas efficient as LiquidityAmounts is used offchain function mint( LiquidityRange calldata position, @@ -18,7 +20,7 @@ interface INonfungiblePositionManager { uint256 deadline, address recipient, bytes calldata hookData - ) external payable returns (BalanceDelta delta); + ) external payable; // NOTE: more expensive since LiquidityAmounts is used onchain // function mint(MintParams calldata params) external payable returns (uint256 tokenId, BalanceDelta delta); @@ -28,20 +30,14 @@ interface INonfungiblePositionManager { /// @param liquidity The amount of liquidity to add /// @param hookData Arbitrary data passed to the hook /// @param claims Whether the liquidity increase uses ERC-6909 claim tokens - /// @return delta Corresponding balance changes as a result of increasing liquidity - function increaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims) - external - returns (BalanceDelta delta); + function increaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims) external; /// @notice Decrease liquidity for an existing position /// @param tokenId The ID of the position /// @param liquidity The amount of liquidity to remove /// @param hookData Arbitrary data passed to the hook /// @param claims Whether the removed liquidity is sent as ERC-6909 claim tokens - /// @return delta Corresponding balance changes as a result of decreasing liquidity applied to user (number of tokens credited to tokensOwed) - function decreaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims) - external - returns (BalanceDelta delta); + function decreaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims) external; // TODO Can decide if we want burn to auto encode a decrease/collect. /// @notice Burn a position and delete the tokenId @@ -56,15 +52,12 @@ interface INonfungiblePositionManager { /// @param recipient The address to send the collected tokens to /// @param hookData Arbitrary data passed to the hook /// @param claims Whether the collected fees are sent as ERC-6909 claim tokens - /// @return delta Corresponding balance changes as a result of collecting fees - function collect(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) - external - returns (BalanceDelta delta); + function collect(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) external; /// @notice Execute a batch of external calls by unlocking the PoolManager /// @param data an array of abi.encodeWithSelector(, ) for each call /// @return delta The final delta changes of the caller - function unlockAndExecute(bytes[] memory data, Currency[] memory currencies) external returns (bytes memory); + function unlockAndExecute(bytes[] memory data, Currency[] memory currencies) external returns (int128[] memory); /// @notice Returns the fees owed for a position. Includes unclaimed fees + custodied fees + claimable fees /// @param tokenId The ID of the position diff --git a/contracts/libraries/TransientLiquidityDelta.sol b/contracts/libraries/TransientLiquidityDelta.sol index 9dca43a5..fdc1e614 100644 --- a/contracts/libraries/TransientLiquidityDelta.sol +++ b/contracts/libraries/TransientLiquidityDelta.sol @@ -25,11 +25,6 @@ library TransientLiquidityDelta { } } - function close(address holder, Currency currency0, Currency currency1) internal { - setDelta(currency0, holder, 0); - setDelta(currency1, holder, 0); - } - /// @notice Flush a BalanceDelta into transient storage for a given holder function flush(BalanceDelta delta, address holder, Currency currency0, Currency currency1) internal { addDelta(currency0, holder, delta.amount0()); @@ -54,19 +49,20 @@ library TransientLiquidityDelta { } } - function close(Currency currency, IPoolManager manager, address holder) internal { + function close(Currency currency, IPoolManager manager, address holder, bool claims) + internal + returns (int128 delta) + { // getDelta(currency, holder); bytes32 hashSlot = _computeSlot(holder, currency); - int256 delta; assembly { delta := tload(hashSlot) } - // TODO support claims field if (delta < 0) { - currency.settle(manager, holder, uint256(-delta), false); + currency.settle(manager, holder, uint256(-int256(delta)), claims); } else { - currency.take(manager, holder, uint256(delta), false); + currency.take(manager, holder, uint256(int256(delta)), claims); } // setDelta(0); @@ -75,6 +71,18 @@ library TransientLiquidityDelta { } } + function closeDelta( + BalanceDelta delta, + IPoolManager manager, + address holder, + Currency currency0, + Currency currency1, + bool claims + ) internal { + close(currency0, manager, holder, claims); + close(currency1, manager, holder, claims); + } + function getBalanceDelta(address holder, Currency currency0, Currency currency1) internal view diff --git a/test/position-managers/Execute.t.sol b/test/position-managers/Execute.t.sol index 5d6b7d39..5cc9eb8e 100644 --- a/test/position-managers/Execute.t.sol +++ b/test/position-managers/Execute.t.sol @@ -27,15 +27,15 @@ import {LiquidityRange, LiquidityRangeId, LiquidityRangeIdLibrary} from "../../c import {LiquidityFuzzers} from "../shared/fuzz/LiquidityFuzzers.sol"; -contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { +import {LiquidityOperations} from "../shared/LiquidityOperations.sol"; + +contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, LiquidityOperations { using FixedPointMathLib for uint256; using CurrencyLibrary for Currency; using LiquidityRangeIdLibrary for LiquidityRange; using PoolIdLibrary for PoolKey; using SafeCast for uint256; - NonfungiblePositionManager lpm; - PoolId poolId; address alice = makeAddr("ALICE"); address bob = makeAddr("BOB"); @@ -79,7 +79,7 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { function test_execute_increaseLiquidity_once(uint256 initialLiquidity, uint256 liquidityToAdd) public { initialLiquidity = bound(initialLiquidity, 1e18, 1000e18); liquidityToAdd = bound(liquidityToAdd, 1e18, 1000e18); - lpm.mint(range, initialLiquidity, 0, address(this), ZERO_BYTES); + _mint(range, initialLiquidity, 0, address(this), ZERO_BYTES); uint256 tokenId = lpm.nextTokenId() - 1; bytes[] memory data = new bytes[](1); @@ -104,7 +104,7 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { initialiLiquidity = bound(initialiLiquidity, 1e18, 1000e18); liquidityToAdd = bound(liquidityToAdd, 1e18, 1000e18); liquidityToAdd2 = bound(liquidityToAdd2, 1e18, 1000e18); - lpm.mint(range, initialiLiquidity, 0, address(this), ZERO_BYTES); + _mint(range, initialiLiquidity, 0, address(this), ZERO_BYTES); uint256 tokenId = lpm.nextTokenId() - 1; bytes[] memory data = new bytes[](2); diff --git a/test/position-managers/FeeCollection.t.sol b/test/position-managers/FeeCollection.t.sol index 1a8071a6..7d3d4716 100644 --- a/test/position-managers/FeeCollection.t.sol +++ b/test/position-managers/FeeCollection.t.sol @@ -10,7 +10,7 @@ import {Deployers} from "@uniswap/v4-core/test/utils/Deployers.sol"; import {Currency, CurrencyLibrary} from "@uniswap/v4-core/src/types/Currency.sol"; import {PoolId, PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol"; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; -import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; +import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; import {PoolSwapTest} from "@uniswap/v4-core/src/test/PoolSwapTest.sol"; import {LiquidityAmounts} from "../../contracts/libraries/LiquidityAmounts.sol"; import {TickMath} from "@uniswap/v4-core/src/libraries/TickMath.sol"; @@ -24,22 +24,19 @@ import {LiquidityRange, LiquidityRangeId, LiquidityRangeIdLibrary} from "../../c import {LiquidityFuzzers} from "../shared/fuzz/LiquidityFuzzers.sol"; -contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { +import {LiquidityOperations} from "../shared/LiquidityOperations.sol"; + +contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, LiquidityOperations { using FixedPointMathLib for uint256; using CurrencyLibrary for Currency; using LiquidityRangeIdLibrary for LiquidityRange; - NonfungiblePositionManager lpm; - PoolId poolId; address alice = makeAddr("ALICE"); address bob = makeAddr("BOB"); uint256 constant STARTING_USER_BALANCE = 10_000_000 ether; - // unused value for the fuzz helper functions - uint128 constant DEAD_VALUE = 6969.6969 ether; - // expresses the fee as a wad (i.e. 3000 = 0.003e18) uint256 FEE_WAD; @@ -69,25 +66,26 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { vm.stopPrank(); } - function test_collect_6909(IPoolManager.ModifyLiquidityParams memory params) public { - params.liquidityDelta = bound(params.liquidityDelta, 10e18, 10_000e18); - uint256 tokenId; - (tokenId, params,) = createFuzzyLiquidity(lpm, address(this), key, params, SQRT_PRICE_1_1, ZERO_BYTES); - vm.assume(params.tickLower < 0 && 0 < params.tickUpper); // require two-sided liquidity + // TODO: we dont accept collecting fees as 6909 yet + // function test_collect_6909(IPoolManager.ModifyLiquidityParams memory params) public { + // params.liquidityDelta = bound(params.liquidityDelta, 10e18, 10_000e18); + // uint256 tokenId; + // (tokenId, params,) = createFuzzyLiquidity(lpm, address(this), key, params, SQRT_PRICE_1_1, ZERO_BYTES); + // vm.assume(params.tickLower < 0 && 0 < params.tickUpper); // require two-sided liquidity - // swap to create fees - uint256 swapAmount = 0.01e18; - swap(key, false, -int256(swapAmount), ZERO_BYTES); + // // swap to create fees + // uint256 swapAmount = 0.01e18; + // swap(key, false, -int256(swapAmount), ZERO_BYTES); - // collect fees - BalanceDelta delta = lpm.collect(tokenId, address(this), ZERO_BYTES, true); + // // collect fees + // BalanceDelta delta = _collect(tokenId, address(this), ZERO_BYTES, true); - assertEq(delta.amount0(), 0); + // assertEq(delta.amount0(), 0); - assertApproxEqAbs(uint256(int256(delta.amount1())), swapAmount.mulWadDown(FEE_WAD), 1 wei); + // assertApproxEqAbs(uint256(int256(delta.amount1())), swapAmount.mulWadDown(FEE_WAD), 1 wei); - assertEq(uint256(int256(delta.amount1())), manager.balanceOf(address(this), currency1.toId())); - } + // assertEq(uint256(int256(delta.amount1())), manager.balanceOf(address(this), currency1.toId())); + // } function test_collect_erc20(IPoolManager.ModifyLiquidityParams memory params) public { params.liquidityDelta = bound(params.liquidityDelta, 10e18, 10_000e18); @@ -102,7 +100,7 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { // collect fees uint256 balance0Before = currency0.balanceOfSelf(); uint256 balance1Before = currency1.balanceOfSelf(); - BalanceDelta delta = lpm.collect(tokenId, address(this), ZERO_BYTES, false); + BalanceDelta delta = _collect(tokenId, address(this), ZERO_BYTES, false); assertEq(delta.amount0(), 0); @@ -112,48 +110,49 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { assertEq(uint256(int256(delta.amount1())), currency1.balanceOfSelf() - balance1Before); } + // TODO: we dont accept collecting fees as 6909 yet // two users with the same range; one user cannot collect the other's fees - function test_collect_sameRange_6909(IPoolManager.ModifyLiquidityParams memory params, uint256 liquidityDeltaBob) - public - { - params.liquidityDelta = bound(params.liquidityDelta, 10e18, 10_000e18); - params = createFuzzyLiquidityParams(key, params, SQRT_PRICE_1_1); - vm.assume(params.tickLower < 0 && 0 < params.tickUpper); // require two-sided liquidity - - liquidityDeltaBob = bound(liquidityDeltaBob, 100e18, 100_000e18); - - LiquidityRange memory range = - LiquidityRange({poolKey: key, tickLower: params.tickLower, tickUpper: params.tickUpper}); - vm.prank(alice); - lpm.mint(range, uint256(params.liquidityDelta), block.timestamp + 1, alice, ZERO_BYTES); - uint256 tokenIdAlice = lpm.nextTokenId() - 1; - - vm.prank(bob); - lpm.mint(range, liquidityDeltaBob, block.timestamp + 1, bob, ZERO_BYTES); - uint256 tokenIdBob = lpm.nextTokenId() - 1; - - // swap to create fees - uint256 swapAmount = 0.01e18; - swap(key, false, -int256(swapAmount), ZERO_BYTES); - - // alice collects only her fees - vm.prank(alice); - BalanceDelta delta = lpm.collect(tokenIdAlice, alice, ZERO_BYTES, true); - assertEq(uint256(uint128(delta.amount0())), manager.balanceOf(alice, currency0.toId())); - assertEq(uint256(uint128(delta.amount1())), manager.balanceOf(alice, currency1.toId())); - assertTrue(delta.amount1() != 0); - - // bob collects only his fees - vm.prank(bob); - delta = lpm.collect(tokenIdBob, bob, ZERO_BYTES, true); - assertEq(uint256(uint128(delta.amount0())), manager.balanceOf(bob, currency0.toId())); - assertEq(uint256(uint128(delta.amount1())), manager.balanceOf(bob, currency1.toId())); - assertTrue(delta.amount1() != 0); - - // position manager holds no fees now - assertApproxEqAbs(manager.balanceOf(address(lpm), currency0.toId()), 0, 1 wei); - assertApproxEqAbs(manager.balanceOf(address(lpm), currency1.toId()), 0, 1 wei); - } + // function test_collect_sameRange_6909(IPoolManager.ModifyLiquidityParams memory params, uint256 liquidityDeltaBob) + // public + // { + // params.liquidityDelta = bound(params.liquidityDelta, 10e18, 10_000e18); + // params = createFuzzyLiquidityParams(key, params, SQRT_PRICE_1_1); + // vm.assume(params.tickLower < 0 && 0 < params.tickUpper); // require two-sided liquidity + + // liquidityDeltaBob = bound(liquidityDeltaBob, 100e18, 100_000e18); + + // LiquidityRange memory range = + // LiquidityRange({poolKey: key, tickLower: params.tickLower, tickUpper: params.tickUpper}); + // vm.prank(alice); + // _mint(range, uint256(params.liquidityDelta), block.timestamp + 1, alice, ZERO_BYTES); + // uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // vm.prank(bob); + // _mint(range, liquidityDeltaBob, block.timestamp + 1, bob, ZERO_BYTES); + // uint256 tokenIdBob = lpm.nextTokenId() - 1; + + // // swap to create fees + // uint256 swapAmount = 0.01e18; + // swap(key, false, -int256(swapAmount), ZERO_BYTES); + + // // alice collects only her fees + // vm.prank(alice); + // BalanceDelta delta = _collect(tokenIdAlice, alice, ZERO_BYTES, true); + // assertEq(uint256(uint128(delta.amount0())), manager.balanceOf(alice, currency0.toId())); + // assertEq(uint256(uint128(delta.amount1())), manager.balanceOf(alice, currency1.toId())); + // assertTrue(delta.amount1() != 0); + + // // bob collects only his fees + // vm.prank(bob); + // delta = _collect(tokenIdBob, bob, ZERO_BYTES, true); + // assertEq(uint256(uint128(delta.amount0())), manager.balanceOf(bob, currency0.toId())); + // assertEq(uint256(uint128(delta.amount1())), manager.balanceOf(bob, currency1.toId())); + // assertTrue(delta.amount1() != 0); + + // // position manager holds no fees now + // assertApproxEqAbs(manager.balanceOf(address(lpm), currency0.toId()), 0, 1 wei); + // assertApproxEqAbs(manager.balanceOf(address(lpm), currency1.toId()), 0, 1 wei); + // } function test_collect_sameRange_erc20(IPoolManager.ModifyLiquidityParams memory params, uint256 liquidityDeltaBob) public @@ -167,11 +166,11 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { LiquidityRange memory range = LiquidityRange({poolKey: key, tickLower: params.tickLower, tickUpper: params.tickUpper}); vm.prank(alice); - lpm.mint(range, uint256(params.liquidityDelta), block.timestamp + 1, alice, ZERO_BYTES); + _mint(range, uint256(params.liquidityDelta), block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; vm.prank(bob); - lpm.mint(range, liquidityDeltaBob, block.timestamp + 1, bob, ZERO_BYTES); + _mint(range, liquidityDeltaBob, block.timestamp + 1, bob, ZERO_BYTES); uint256 tokenIdBob = lpm.nextTokenId() - 1; // confirm the positions are same range @@ -187,8 +186,9 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { // alice collects only her fees uint256 balance0AliceBefore = currency0.balanceOf(alice); uint256 balance1AliceBefore = currency1.balanceOf(alice); - vm.prank(alice); - BalanceDelta delta = lpm.collect(tokenIdAlice, alice, ZERO_BYTES, false); + vm.startPrank(alice); + BalanceDelta delta = _collect(tokenIdAlice, alice, ZERO_BYTES, false); + vm.stopPrank(); uint256 balance0AliceAfter = currency0.balanceOf(alice); uint256 balance1AliceAfter = currency1.balanceOf(alice); @@ -199,8 +199,9 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { // bob collects only his fees uint256 balance0BobBefore = currency0.balanceOf(bob); uint256 balance1BobBefore = currency1.balanceOf(bob); - vm.prank(bob); - delta = lpm.collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.startPrank(bob); + delta = _collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.stopPrank(); uint256 balance0BobAfter = currency0.balanceOf(bob); uint256 balance1BobAfter = currency1.balanceOf(bob); @@ -228,11 +229,11 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { uint256 liquidityAlice = 3000e18; uint256 liquidityBob = 1000e18; vm.prank(alice); - BalanceDelta lpDeltaAlice = lpm.mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + BalanceDelta lpDeltaAlice = _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; vm.prank(bob); - BalanceDelta lpDeltaBob = lpm.mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); + BalanceDelta lpDeltaBob = _mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); uint256 tokenIdBob = lpm.nextTokenId() - 1; // swap to create fees @@ -242,7 +243,7 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { // alice decreases liquidity vm.prank(alice); - BalanceDelta aliceDelta = lpm.decreaseLiquidity(tokenIdAlice, liquidityAlice, ZERO_BYTES, true); + _decreaseLiquidity(tokenIdAlice, liquidityAlice, ZERO_BYTES, true); uint256 tolerance = 0.000000001 ether; @@ -259,7 +260,7 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { // bob decreases half of his liquidity vm.prank(bob); - BalanceDelta bobDelta = lpm.decreaseLiquidity(tokenIdBob, liquidityBob / 2, ZERO_BYTES, true); + _decreaseLiquidity(tokenIdBob, liquidityBob / 2, ZERO_BYTES, true); // lpm collects half of bobs principal // the fee amount has already been collected with alice's calls diff --git a/test/position-managers/Gas.t.sol b/test/position-managers/Gas.t.sol index e25d85f7..3d87fd77 100644 --- a/test/position-managers/Gas.t.sol +++ b/test/position-managers/Gas.t.sol @@ -23,23 +23,20 @@ import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import {NonfungiblePositionManager} from "../../contracts/NonfungiblePositionManager.sol"; import {LiquidityRange, LiquidityRangeId, LiquidityRangeIdLibrary} from "../../contracts/types/LiquidityRange.sol"; -contract GasTest is Test, Deployers, GasSnapshot { +import {LiquidityOperations} from "../shared/LiquidityOperations.sol"; + +contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { using FixedPointMathLib for uint256; using CurrencyLibrary for Currency; using LiquidityRangeIdLibrary for LiquidityRange; using PoolIdLibrary for PoolKey; - NonfungiblePositionManager lpm; - PoolId poolId; address alice = makeAddr("ALICE"); address bob = makeAddr("BOB"); uint256 constant STARTING_USER_BALANCE = 10_000_000 ether; - // unused value for the fuzz helper functions - uint128 constant DEAD_VALUE = 6969.6969 ether; - // expresses the fee as a wad (i.e. 3000 = 0.003e18 = 0.30%) uint256 FEE_WAD; @@ -98,23 +95,42 @@ contract GasTest is Test, Deployers, GasSnapshot { // } function test_gas_mintWithLiquidity() public { - lpm.mint(range, 10_000 ether, block.timestamp + 1, address(this), ZERO_BYTES); + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector( + lpm.mint.selector, range, 10_000 ether, block.timestamp + 1, address(this), ZERO_BYTES + ); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + lpm.unlockAndExecute(calls, currencies); snapLastCall("mintWithLiquidity"); } function test_gas_increaseLiquidity_erc20() public { - lpm.mint(range, 10_000 ether, block.timestamp + 1, address(this), ZERO_BYTES); + _mint(range, 10_000 ether, block.timestamp + 1, address(this), ZERO_BYTES); uint256 tokenId = lpm.nextTokenId() - 1; - lpm.increaseLiquidity(tokenId, 1000 ether, ZERO_BYTES, false); + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector(lpm.increaseLiquidity.selector, tokenId, 10_000 ether, ZERO_BYTES, false); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + + lpm.unlockAndExecute(calls, currencies); snapLastCall("increaseLiquidity_erc20"); } function test_gas_increaseLiquidity_erc6909() public { - lpm.mint(range, 10_000 ether, block.timestamp + 1, address(this), ZERO_BYTES); + _mint(range, 10_000 ether, block.timestamp + 1, address(this), ZERO_BYTES); uint256 tokenId = lpm.nextTokenId() - 1; - lpm.increaseLiquidity(tokenId, 1000 ether, ZERO_BYTES, true); + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector(lpm.increaseLiquidity.selector, tokenId, 10_000 ether, ZERO_BYTES, true); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + + lpm.unlockAndExecute(calls, currencies); snapLastCall("increaseLiquidity_erc6909"); } @@ -127,12 +143,12 @@ contract GasTest is Test, Deployers, GasSnapshot { // alice provides liquidity vm.prank(alice); - lpm.mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob provides liquidity vm.prank(bob); - lpm.mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); + _mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); // donate to create fees donateRouter.donate(key, 0.2e18, 0.2e18, ZERO_BYTES); @@ -149,8 +165,15 @@ contract GasTest is Test, Deployers, GasSnapshot { token1Owed ); + bytes[] memory calls = new bytes[](1); + calls[0] = + abi.encodeWithSelector(lpm.increaseLiquidity.selector, tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + vm.prank(alice); - lpm.increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + lpm.unlockAndExecute(calls, currencies); snapLastCall("autocompound_exactUnclaimedFees"); } @@ -162,20 +185,26 @@ contract GasTest is Test, Deployers, GasSnapshot { // alice provides liquidity vm.prank(alice); - lpm.mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob provides liquidity vm.prank(bob); - lpm.mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); + _mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); uint256 tokenIdBob = lpm.nextTokenId() - 1; // donate to create fees donateRouter.donate(key, 20e18, 20e18, ZERO_BYTES); // bob collects fees so some of alice's fees are now cached + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector(lpm.collect.selector, tokenIdBob, bob, ZERO_BYTES, false); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + vm.prank(bob); - lpm.collect(tokenIdBob, bob, ZERO_BYTES, false); + lpm.unlockAndExecute(calls, currencies); // donate to create more fees donateRouter.donate(key, 20e18, 20e18, ZERO_BYTES); @@ -193,8 +222,15 @@ contract GasTest is Test, Deployers, GasSnapshot { newToken1Owed ); + calls = new bytes[](1); + calls[0] = + abi.encodeWithSelector(lpm.increaseLiquidity.selector, tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + vm.prank(alice); - lpm.increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + lpm.unlockAndExecute(calls, currencies); snapLastCall("autocompound_exactUnclaimedFees_exactCustodiedFees"); } } @@ -208,12 +244,12 @@ contract GasTest is Test, Deployers, GasSnapshot { // alice provides liquidity vm.prank(alice); - lpm.mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob provides liquidity vm.prank(bob); - lpm.mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); + _mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); uint256 tokenIdBob = lpm.nextTokenId() - 1; // donate to create fees @@ -231,24 +267,43 @@ contract GasTest is Test, Deployers, GasSnapshot { token1Owed / 2 ); + bytes[] memory calls = new bytes[](1); + calls[0] = + abi.encodeWithSelector(lpm.increaseLiquidity.selector, tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + vm.prank(alice); - lpm.increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + lpm.unlockAndExecute(calls, currencies); snapLastCall("autocompound_excessFeesCredit"); } function test_gas_decreaseLiquidity_erc20() public { - lpm.mint(range, 10_000 ether, block.timestamp + 1, address(this), ZERO_BYTES); + _mint(range, 10_000 ether, block.timestamp + 1, address(this), ZERO_BYTES); uint256 tokenId = lpm.nextTokenId() - 1; - lpm.decreaseLiquidity(tokenId, 10_000 ether, ZERO_BYTES, false); + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector(lpm.decreaseLiquidity.selector, tokenId, 10_000 ether, ZERO_BYTES, false); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + + lpm.unlockAndExecute(calls, currencies); snapLastCall("decreaseLiquidity_erc20"); } function test_gas_decreaseLiquidity_erc6909() public { - lpm.mint(range, 10_000 ether, block.timestamp + 1, address(this), ZERO_BYTES); + _mint(range, 10_000 ether, block.timestamp + 1, address(this), ZERO_BYTES); uint256 tokenId = lpm.nextTokenId() - 1; - lpm.decreaseLiquidity(tokenId, 10_000 ether, ZERO_BYTES, true); + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector(lpm.decreaseLiquidity.selector, tokenId, 10_000 ether, ZERO_BYTES, true); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + + lpm.unlockAndExecute(calls, currencies); snapLastCall("decreaseLiquidity_erc6909"); } diff --git a/test/position-managers/IncreaseLiquidity.t.sol b/test/position-managers/IncreaseLiquidity.t.sol index 2f6a8a7b..39a6e329 100644 --- a/test/position-managers/IncreaseLiquidity.t.sol +++ b/test/position-managers/IncreaseLiquidity.t.sol @@ -10,7 +10,7 @@ import {Deployers} from "@uniswap/v4-core/test/utils/Deployers.sol"; import {Currency, CurrencyLibrary} from "@uniswap/v4-core/src/types/Currency.sol"; import {PoolId, PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol"; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; -import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; +import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; import {PoolSwapTest} from "@uniswap/v4-core/src/test/PoolSwapTest.sol"; import {LiquidityAmounts} from "../../contracts/libraries/LiquidityAmounts.sol"; import {TickMath} from "@uniswap/v4-core/src/libraries/TickMath.sol"; @@ -25,23 +25,20 @@ import {LiquidityRange, LiquidityRangeId, LiquidityRangeIdLibrary} from "../../c import {Fuzzers} from "@uniswap/v4-core/src/test/Fuzzers.sol"; -contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { +import {LiquidityOperations} from "../shared/LiquidityOperations.sol"; + +contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperations { using FixedPointMathLib for uint256; using CurrencyLibrary for Currency; using LiquidityRangeIdLibrary for LiquidityRange; using PoolIdLibrary for PoolKey; - NonfungiblePositionManager lpm; - PoolId poolId; address alice = makeAddr("ALICE"); address bob = makeAddr("BOB"); uint256 constant STARTING_USER_BALANCE = 10_000_000 ether; - // unused value for the fuzz helper functions - uint128 constant DEAD_VALUE = 6969.6969 ether; - // expresses the fee as a wad (i.e. 3000 = 0.003e18 = 0.30%) uint256 FEE_WAD; @@ -85,12 +82,12 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { // alice provides liquidity vm.prank(alice); - lpm.mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob provides liquidity vm.prank(bob); - lpm.mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); + _mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); // swap to create fees uint256 swapAmount = 0.001e18; @@ -112,8 +109,9 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { uint256 balance0BeforeAlice = currency0.balanceOf(alice); uint256 balance1BeforeAlice = currency1.balanceOf(alice); - vm.prank(alice); - lpm.increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.startPrank(alice); + _increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.stopPrank(); // alice did not spend any tokens assertEq(balance0BeforeAlice, currency0.balanceOf(alice)); @@ -135,12 +133,12 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { // alice provides liquidity vm.prank(alice); - lpm.mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob provides liquidity vm.prank(bob); - lpm.mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); + _mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); // donate to create fees donateRouter.donate(key, 0.2e18, 0.2e18, ZERO_BYTES); @@ -160,8 +158,9 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { uint256 balance0BeforeAlice = currency0.balanceOf(alice); uint256 balance1BeforeAlice = currency1.balanceOf(alice); - vm.prank(alice); - lpm.increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.startPrank(alice); + _increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.stopPrank(); // alice did not spend any tokens assertEq(balance0BeforeAlice, currency0.balanceOf(alice)); @@ -182,12 +181,12 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { // alice provides liquidity vm.prank(alice); - lpm.mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob provides liquidity vm.prank(bob); - lpm.mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); + _mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); uint256 tokenIdBob = lpm.nextTokenId() - 1; // swap to create fees @@ -207,16 +206,18 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { token1Owed / 2 ); - vm.prank(alice); - lpm.increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.startPrank(alice); + _increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.stopPrank(); } { // bob collects his fees uint256 balance0BeforeBob = currency0.balanceOf(bob); uint256 balance1BeforeBob = currency1.balanceOf(bob); - vm.prank(bob); - lpm.collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.startPrank(bob); + _collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.stopPrank(); uint256 balance0AfterBob = currency0.balanceOf(bob); uint256 balance1AfterBob = currency1.balanceOf(bob); assertApproxEqAbs( @@ -235,8 +236,9 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { // alice collects her fees, which should be about half of the fees uint256 balance0BeforeAlice = currency0.balanceOf(alice); uint256 balance1BeforeAlice = currency1.balanceOf(alice); - vm.prank(alice); - lpm.collect(tokenIdAlice, alice, ZERO_BYTES, false); + vm.startPrank(alice); + _collect(tokenIdAlice, alice, ZERO_BYTES, false); + vm.stopPrank(); uint256 balance0AfterAlice = currency0.balanceOf(alice); uint256 balance1AfterAlice = currency1.balanceOf(alice); assertApproxEqAbs( @@ -261,12 +263,12 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { // alice provides liquidity vm.prank(alice); - lpm.mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob provides liquidity vm.prank(bob); - lpm.mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); + _mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); uint256 tokenIdBob = lpm.nextTokenId() - 1; // swap to create fees @@ -288,8 +290,9 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { uint256 balance0BeforeAlice = currency0.balanceOf(alice); uint256 balance1BeforeAlice = currency1.balanceOf(alice); - vm.prank(alice); - lpm.increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.startPrank(alice); + _increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.stopPrank(); uint256 balance0AfterAlice = currency0.balanceOf(alice); uint256 balance1AfterAlice = currency1.balanceOf(alice); @@ -301,8 +304,9 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { // bob collects his fees uint256 balance0BeforeBob = currency0.balanceOf(bob); uint256 balance1BeforeBob = currency1.balanceOf(bob); - vm.prank(bob); - lpm.collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.startPrank(bob); + _collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.stopPrank(); uint256 balance0AfterBob = currency0.balanceOf(bob); uint256 balance1AfterBob = currency1.balanceOf(bob); assertApproxEqAbs( @@ -327,12 +331,12 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { // alice provides liquidity vm.prank(alice); - lpm.mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob provides liquidity vm.prank(bob); - lpm.mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); + _mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); uint256 tokenIdBob = lpm.nextTokenId() - 1; // swap to create fees @@ -343,8 +347,9 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { (uint256 token0Owed, uint256 token1Owed) = lpm.feesOwed(tokenIdAlice); // bob collects fees so some of alice's fees are now cached - vm.prank(bob); - lpm.collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.startPrank(bob); + _collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.stopPrank(); // swap to create more fees swap(key, true, -int256(swapAmount), ZERO_BYTES); @@ -369,8 +374,9 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { newToken1Owed ); - vm.prank(alice); - lpm.increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.startPrank(alice); + _increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.stopPrank(); } // alice did not spend any tokens @@ -393,12 +399,12 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { // alice provides liquidity vm.prank(alice); - lpm.mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob provides liquidity vm.prank(bob); - lpm.mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); + _mint(range, liquidityBob, block.timestamp + 1, bob, ZERO_BYTES); uint256 tokenIdBob = lpm.nextTokenId() - 1; // donate to create fees @@ -407,8 +413,9 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { (uint256 token0Owed, uint256 token1Owed) = lpm.feesOwed(tokenIdAlice); // bob collects fees so some of alice's fees are now cached - vm.prank(bob); - lpm.collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.startPrank(bob); + _collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.stopPrank(); // donate to create more fees donateRouter.donate(key, 20e18, 20e18, ZERO_BYTES); @@ -432,8 +439,9 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { newToken1Owed ); - vm.prank(alice); - lpm.increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.startPrank(alice); + _increaseLiquidity(tokenIdAlice, liquidityDelta, ZERO_BYTES, false); + vm.stopPrank(); } // alice did not spend any tokens @@ -449,8 +457,9 @@ contract IncreaseLiquidityTest is Test, Deployers, GasSnapshot, Fuzzers { assertApproxEqAbs(token0Owed, 5e18, 1 wei); assertApproxEqAbs(token1Owed, 5e18, 1 wei); - vm.prank(bob); - BalanceDelta result = lpm.collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.startPrank(bob); + BalanceDelta result = _collect(tokenIdBob, bob, ZERO_BYTES, false); + vm.stopPrank(); assertApproxEqAbs(result.amount0(), 5e18, 1 wei); assertApproxEqAbs(result.amount1(), 5e18, 1 wei); } diff --git a/test/position-managers/NonfungiblePositionManager.t.sol b/test/position-managers/NonfungiblePositionManager.t.sol index 4b8a7791..0c4f52bc 100644 --- a/test/position-managers/NonfungiblePositionManager.t.sol +++ b/test/position-managers/NonfungiblePositionManager.t.sol @@ -10,7 +10,7 @@ import {Deployers} from "@uniswap/v4-core/test/utils/Deployers.sol"; import {Currency, CurrencyLibrary} from "@uniswap/v4-core/src/types/Currency.sol"; import {PoolId, PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol"; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; -import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; +import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; import {PoolSwapTest} from "@uniswap/v4-core/src/test/PoolSwapTest.sol"; import {LiquidityAmounts} from "../../contracts/libraries/LiquidityAmounts.sol"; import {TickMath} from "@uniswap/v4-core/src/libraries/TickMath.sol"; @@ -24,19 +24,16 @@ import {LiquidityRange, LiquidityRangeId, LiquidityRangeIdLibrary} from "../../c import {LiquidityFuzzers} from "../shared/fuzz/LiquidityFuzzers.sol"; -contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, LiquidityFuzzers { +import {LiquidityOperations} from "../shared/LiquidityOperations.sol"; + +contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, LiquidityOperations { using FixedPointMathLib for uint256; using CurrencyLibrary for Currency; using LiquidityRangeIdLibrary for LiquidityRange; - NonfungiblePositionManager lpm; - PoolId poolId; address alice = makeAddr("ALICE"); - // unused value for the fuzz helper functions - uint128 constant DEAD_VALUE = 6969.6969 ether; - function setUp() public { Deployers.deployFreshManagerAndRouters(); Deployers.deployMintAndApprove2Currencies(); @@ -56,8 +53,17 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi uint256 balance0Before = currency0.balanceOfSelf(); uint256 balance1Before = currency1.balanceOfSelf(); - BalanceDelta delta = - lpm.mint(range, uint256(params.liquidityDelta), block.timestamp + 1, address(this), ZERO_BYTES); + + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector( + lpm.mint.selector, range, uint256(params.liquidityDelta), block.timestamp + 1, address(this), ZERO_BYTES + ); + Currency[] memory currencies = new Currency[](2); + currencies[0] = currency0; + currencies[1] = currency1; + int128[] memory result = lpm.unlockAndExecute(calls, currencies); + BalanceDelta delta = toBalanceDelta(result[0], result[1]); + uint256 balance0After = currency0.balanceOfSelf(); uint256 balance1After = currency1.balanceOfSelf(); @@ -216,8 +222,8 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi uint256 balance0BeforeBurn = currency0.balanceOfSelf(); uint256 balance1BeforeBurn = currency1.balanceOfSelf(); // TODO, encode this under one call - lpm.decreaseLiquidity(tokenId, liquidity, ZERO_BYTES, false); - lpm.collect(tokenId, address(this), ZERO_BYTES, false); + _decreaseLiquidity(tokenId, liquidity, ZERO_BYTES, false); + _collect(tokenId, address(this), ZERO_BYTES, false); BalanceDelta delta = lpm.burn(tokenId); (,, liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, 0); @@ -249,7 +255,7 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi uint256 balance0Before = currency0.balanceOfSelf(); uint256 balance1Before = currency1.balanceOfSelf(); - BalanceDelta delta = lpm.decreaseLiquidity(tokenId, decreaseLiquidityDelta, ZERO_BYTES, false); + BalanceDelta delta = _decreaseLiquidity(tokenId, decreaseLiquidityDelta, ZERO_BYTES, false); (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, uint256(params.liquidityDelta) - decreaseLiquidityDelta); diff --git a/test/shared/LiquidityOperations.sol b/test/shared/LiquidityOperations.sol new file mode 100644 index 00000000..a3d556cd --- /dev/null +++ b/test/shared/LiquidityOperations.sol @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; +import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; + +import {NonfungiblePositionManager} from "../../contracts/NonfungiblePositionManager.sol"; +import {LiquidityRange} from "../../contracts/types/LiquidityRange.sol"; + +contract LiquidityOperations { + NonfungiblePositionManager lpm; + + function _mint( + LiquidityRange memory _range, + uint256 liquidity, + uint256 deadline, + address recipient, + bytes memory hookData + ) internal returns (BalanceDelta) { + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector(lpm.mint.selector, _range, liquidity, deadline, recipient, hookData); + Currency[] memory currencies = new Currency[](2); + currencies[0] = _range.poolKey.currency0; + currencies[1] = _range.poolKey.currency1; + int128[] memory result = lpm.unlockAndExecute(calls, currencies); + return toBalanceDelta(result[0], result[1]); + } + + function _increaseLiquidity(uint256 tokenId, uint256 liquidityToAdd, bytes memory hookData, bool claims) internal { + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector(lpm.increaseLiquidity.selector, tokenId, liquidityToAdd, hookData, claims); + + (, LiquidityRange memory _range) = lpm.tokenPositions(tokenId); + + Currency[] memory currencies = new Currency[](2); + currencies[0] = _range.poolKey.currency0; + currencies[1] = _range.poolKey.currency1; + lpm.unlockAndExecute(calls, currencies); + } + + function _decreaseLiquidity(uint256 tokenId, uint256 liquidityToRemove, bytes memory hookData, bool claims) + internal + returns (BalanceDelta) + { + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector(lpm.decreaseLiquidity.selector, tokenId, liquidityToRemove, hookData, claims); + + (, LiquidityRange memory _range) = lpm.tokenPositions(tokenId); + + Currency[] memory currencies = new Currency[](2); + currencies[0] = _range.poolKey.currency0; + currencies[1] = _range.poolKey.currency1; + int128[] memory result = lpm.unlockAndExecute(calls, currencies); + return toBalanceDelta(result[0], result[1]); + } + + function _collect(uint256 tokenId, address recipient, bytes memory hookData, bool claims) + internal + returns (BalanceDelta) + { + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector(lpm.collect.selector, tokenId, recipient, hookData, claims); + + (, LiquidityRange memory _range) = lpm.tokenPositions(tokenId); + + Currency[] memory currencies = new Currency[](2); + currencies[0] = _range.poolKey.currency0; + currencies[1] = _range.poolKey.currency1; + int128[] memory result = lpm.unlockAndExecute(calls, currencies); + return toBalanceDelta(result[0], result[1]); + } +} diff --git a/test/shared/fuzz/LiquidityFuzzers.sol b/test/shared/fuzz/LiquidityFuzzers.sol index e118e062..de4fe12f 100644 --- a/test/shared/fuzz/LiquidityFuzzers.sol +++ b/test/shared/fuzz/LiquidityFuzzers.sol @@ -3,7 +3,8 @@ pragma solidity ^0.8.24; import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; -import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; +import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; +import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; import {Fuzzers} from "@uniswap/v4-core/src/test/Fuzzers.sol"; import {INonfungiblePositionManager} from "../../../contracts/interfaces/INonfungiblePositionManager.sol"; @@ -20,13 +21,21 @@ contract LiquidityFuzzers is Fuzzers { ) internal returns (uint256, IPoolManager.ModifyLiquidityParams memory, BalanceDelta) { params = Fuzzers.createFuzzyLiquidityParams(key, params, sqrtPriceX96); - BalanceDelta delta = lpm.mint( - LiquidityRange({poolKey: key, tickLower: params.tickLower, tickUpper: params.tickUpper}), - uint256(params.liquidityDelta), - block.timestamp, - recipient, - hookData + LiquidityRange memory range = + LiquidityRange({poolKey: key, tickLower: params.tickLower, tickUpper: params.tickUpper}); + + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector( + lpm.mint.selector, range, uint256(params.liquidityDelta), block.timestamp, recipient, hookData ); + + Currency[] memory currencies = new Currency[](2); + currencies[0] = key.currency0; + currencies[1] = key.currency1; + + int128[] memory result = lpm.unlockAndExecute(calls, currencies); + BalanceDelta delta = toBalanceDelta(result[0], result[1]); + uint256 tokenId = lpm.nextTokenId() - 1; return (tokenId, params, delta); } From 595bd874fb89bdbef1500b3978f97fd056fdaac6 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sat, 6 Jul 2024 21:36:57 -0400 Subject: [PATCH 08/14] move operator to NFTposm; move nonce to ERC721Permit; owner-level nonce --- contracts/NonfungiblePositionManager.sol | 17 +++++++----- contracts/base/ERC721Permit.sol | 26 ++++++++++++------- .../interfaces/IBaseLiquidityManagement.sol | 4 --- .../INonfungiblePositionManager.sol | 1 + test/position-managers/Execute.t.sol | 6 ++--- test/position-managers/FeeCollection.t.sol | 4 +-- .../NonfungiblePositionManager.t.sol | 10 +++---- 7 files changed, 39 insertions(+), 29 deletions(-) diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index e6be1e49..3bf4be9e 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -96,7 +96,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit // mint receipt token uint256 tokenId; _mint(owner, (tokenId = nextTokenId++)); - tokenPositions[tokenId] = TokenPosition({owner: owner, range: range}); + tokenPositions[tokenId] = TokenPosition({owner: owner, range: range, operator: address(0x0)}); } // NOTE: more expensive since LiquidityAmounts is used onchain @@ -170,19 +170,24 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit TokenPosition storage tokenPosition = tokenPositions[tokenId]; LiquidityRangeId rangeId = tokenPosition.range.toId(); Position storage position = positions[from][rangeId]; - position.operator = address(0x0); // transfer position data to destination positions[to][rangeId] = position; delete positions[from][rangeId]; // update token position - tokenPositions[tokenId] = TokenPosition({owner: to, range: tokenPosition.range}); + tokenPositions[tokenId] = TokenPosition({owner: to, range: tokenPosition.range, operator: address(0x0)}); } - function _getAndIncrementNonce(uint256 tokenId) internal override returns (uint256) { - TokenPosition memory tokenPosition = tokenPositions[tokenId]; - return uint256(positions[tokenPosition.owner][tokenPosition.range.toId()].nonce++); + // override ERC721 approval by setting operator + function _approve(address spender, uint256 tokenId) internal override { + tokenPositions[tokenId].operator = spender; + } + + function getApproved(uint256 tokenId) public view override returns (address) { + require(_exists(tokenId), "ERC721: approved query for nonexistent token"); + + return tokenPositions[tokenId].operator; } modifier isAuthorizedForToken(uint256 tokenId) { diff --git a/contracts/base/ERC721Permit.sol b/contracts/base/ERC721Permit.sol index 8eb86521..8323c77e 100644 --- a/contracts/base/ERC721Permit.sol +++ b/contracts/base/ERC721Permit.sol @@ -11,8 +11,7 @@ import {IERC1271} from "../interfaces/external/IERC1271.sol"; /// @title ERC721 with permit /// @notice Nonfungible tokens that support an approve via signature, i.e. permit abstract contract ERC721Permit is ERC721, IERC721Permit { - /// @dev Gets the current nonce for a token ID and then increments it, returning the original value - function _getAndIncrementNonce(uint256 tokenId) internal virtual returns (uint256); + mapping(address owner => uint256 nonce) public nonce; /// @dev The hash of the name used in the permit signature verification bytes32 private immutable nameHash; @@ -53,16 +52,11 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { { require(block.timestamp <= deadline, "Permit expired"); - bytes32 digest = keccak256( - abi.encodePacked( - "\x19\x01", - DOMAIN_SEPARATOR(), - keccak256(abi.encode(PERMIT_TYPEHASH, spender, tokenId, _getAndIncrementNonce(tokenId), deadline)) - ) - ); address owner = ownerOf(tokenId); require(spender != owner, "ERC721Permit: approval to current owner"); + bytes32 digest = getDigest(spender, tokenId, nonce[owner]++, deadline); + if (Address.isContract(owner)) { require(IERC1271(owner).isValidSignature(digest, abi.encodePacked(r, s, v)) == 0x1626ba7e, "Unauthorized"); } else { @@ -73,4 +67,18 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { approve(spender, tokenId); } + + function getDigest(address spender, uint256 tokenId, uint256 _nonce, uint256 deadline) + public + view + returns (bytes32 digest) + { + digest = keccak256( + abi.encodePacked( + "\x19\x01", + DOMAIN_SEPARATOR(), + keccak256(abi.encode(PERMIT_TYPEHASH, spender, tokenId, _nonce, deadline)) + ) + ); + } } diff --git a/contracts/interfaces/IBaseLiquidityManagement.sol b/contracts/interfaces/IBaseLiquidityManagement.sol index 6bcb6e5b..095021e6 100644 --- a/contracts/interfaces/IBaseLiquidityManagement.sol +++ b/contracts/interfaces/IBaseLiquidityManagement.sol @@ -11,10 +11,6 @@ interface IBaseLiquidityManagement { // details about the liquidity position struct Position { - // the nonce for permits - uint96 nonce; - // the address that is approved for spending this token - address operator; uint256 liquidity; // the fee growth of the aggregate position as of the last action on the individual position uint256 feeGrowthInside0LastX128; diff --git a/contracts/interfaces/INonfungiblePositionManager.sol b/contracts/interfaces/INonfungiblePositionManager.sol index 94cf9206..8cf63f4b 100644 --- a/contracts/interfaces/INonfungiblePositionManager.sol +++ b/contracts/interfaces/INonfungiblePositionManager.sol @@ -9,6 +9,7 @@ interface INonfungiblePositionManager { struct TokenPosition { address owner; LiquidityRange range; + address operator; } error MustBeUnlockedByThisContract(); diff --git a/test/position-managers/Execute.t.sol b/test/position-managers/Execute.t.sol index 5cc9eb8e..5e0d10da 100644 --- a/test/position-managers/Execute.t.sol +++ b/test/position-managers/Execute.t.sol @@ -92,7 +92,7 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, Liquidit currencies[1] = currency1; lpm.unlockAndExecute(data, currencies); - (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); + (uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, initialLiquidity + liquidityToAdd); } @@ -120,7 +120,7 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, Liquidit currencies[1] = currency1; lpm.unlockAndExecute(data, currencies); - (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); + (uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, initialiLiquidity + liquidityToAdd + liquidityToAdd2); } @@ -148,7 +148,7 @@ contract ExecuteTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, Liquidit currencies[1] = currency1; lpm.unlockAndExecute(data, currencies); - (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); + (uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, intialLiquidity + liquidityToAdd); } diff --git a/test/position-managers/FeeCollection.t.sol b/test/position-managers/FeeCollection.t.sol index 7d3d4716..af6c5da7 100644 --- a/test/position-managers/FeeCollection.t.sol +++ b/test/position-managers/FeeCollection.t.sol @@ -174,8 +174,8 @@ contract FeeCollectionTest is Test, Deployers, GasSnapshot, LiquidityFuzzers, Li uint256 tokenIdBob = lpm.nextTokenId() - 1; // confirm the positions are same range - (, LiquidityRange memory rangeAlice) = lpm.tokenPositions(tokenIdAlice); - (, LiquidityRange memory rangeBob) = lpm.tokenPositions(tokenIdBob); + (, LiquidityRange memory rangeAlice,) = lpm.tokenPositions(tokenIdAlice); + (, LiquidityRange memory rangeBob,) = lpm.tokenPositions(tokenIdBob); assertEq(rangeAlice.tickLower, rangeBob.tickLower); assertEq(rangeAlice.tickUpper, rangeBob.tickUpper); diff --git a/test/position-managers/NonfungiblePositionManager.t.sol b/test/position-managers/NonfungiblePositionManager.t.sol index 0c4f52bc..c2703adf 100644 --- a/test/position-managers/NonfungiblePositionManager.t.sol +++ b/test/position-managers/NonfungiblePositionManager.t.sol @@ -68,7 +68,7 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi uint256 balance1After = currency1.balanceOfSelf(); assertEq(lpm.ownerOf(1), address(this)); - (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); + (uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, uint256(params.liquidityDelta)); assertEq(balance0Before - balance0After, uint256(int256(-delta.amount0())), "incorrect amount0"); assertEq(balance1Before - balance1After, uint256(int256(-delta.amount1())), "incorrect amount1"); @@ -215,7 +215,7 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi LiquidityRange({poolKey: key, tickLower: params.tickLower, tickUpper: params.tickUpper}); assertEq(tokenId, 1); assertEq(lpm.ownerOf(1), address(this)); - (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); + (uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, uint256(params.liquidityDelta)); // burn liquidity @@ -225,7 +225,7 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi _decreaseLiquidity(tokenId, liquidity, ZERO_BYTES, false); _collect(tokenId, address(this), ZERO_BYTES, false); BalanceDelta delta = lpm.burn(tokenId); - (,, liquidity,,,,) = lpm.positions(address(this), range.toId()); + (liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, 0); // TODO: slightly off by 1 bip (0.0001%) @@ -257,7 +257,7 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi uint256 balance1Before = currency1.balanceOfSelf(); BalanceDelta delta = _decreaseLiquidity(tokenId, decreaseLiquidityDelta, ZERO_BYTES, false); - (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); + (uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); assertEq(liquidity, uint256(params.liquidityDelta) - decreaseLiquidityDelta); assertEq(currency0.balanceOfSelf() - balance0Before, uint256(int256(delta.amount0()))); @@ -284,7 +284,7 @@ contract NonfungiblePositionManagerTest is Test, Deployers, GasSnapshot, Liquidi // uint256 balance0Before = currency0.balanceOfSelf(); // uint256 balance1Before = currency1.balanceOfSelf(); // BalanceDelta delta = lpm.decreaseLiquidity(tokenId, decreaseLiquidityDelta, ZERO_BYTES, false); - // (,, uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); + // (uint256 liquidity,,,,) = lpm.positions(address(this), range.toId()); // assertEq(liquidity, uint256(params.liquidityDelta) - decreaseLiquidityDelta); // // express key.fee as wad (i.e. 3000 = 0.003e18) From 0f59d0feaa27d73510adc2b6877bccc991012290 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sat, 6 Jul 2024 21:37:17 -0400 Subject: [PATCH 09/14] tests for operator/permit --- test/position-managers/Permit.t.sol | 223 ++++++++++++++++++++++++++++ test/shared/LiquidityOperations.sol | 57 ++++++- 2 files changed, 275 insertions(+), 5 deletions(-) create mode 100644 test/position-managers/Permit.t.sol diff --git a/test/position-managers/Permit.t.sol b/test/position-managers/Permit.t.sol new file mode 100644 index 00000000..51295af5 --- /dev/null +++ b/test/position-managers/Permit.t.sol @@ -0,0 +1,223 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "forge-std/Test.sol"; +import {GasSnapshot} from "forge-gas-snapshot/GasSnapshot.sol"; +import {PoolManager} from "@uniswap/v4-core/src/PoolManager.sol"; +import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; +import {IHooks} from "@uniswap/v4-core/src/interfaces/IHooks.sol"; +import {Deployers} from "@uniswap/v4-core/test/utils/Deployers.sol"; +import {Currency, CurrencyLibrary} from "@uniswap/v4-core/src/types/Currency.sol"; +import {PoolId, PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol"; +import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; +import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; +import {PoolSwapTest} from "@uniswap/v4-core/src/test/PoolSwapTest.sol"; +import {LiquidityAmounts} from "../../contracts/libraries/LiquidityAmounts.sol"; +import {TickMath} from "@uniswap/v4-core/src/libraries/TickMath.sol"; +import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol"; +import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol"; + +import {IERC20} from "forge-std/interfaces/IERC20.sol"; +import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; + +import {NonfungiblePositionManager} from "../../contracts/NonfungiblePositionManager.sol"; +import {LiquidityRange, LiquidityRangeId, LiquidityRangeIdLibrary} from "../../contracts/types/LiquidityRange.sol"; + +import {Fuzzers} from "@uniswap/v4-core/src/test/Fuzzers.sol"; + +import {LiquidityOperations} from "../shared/LiquidityOperations.sol"; + +contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperations { + using FixedPointMathLib for uint256; + using CurrencyLibrary for Currency; + using LiquidityRangeIdLibrary for LiquidityRange; + using PoolIdLibrary for PoolKey; + + PoolId poolId; + address alice; + uint256 alicePK; + address bob; + uint256 bobPK; + + uint256 constant STARTING_USER_BALANCE = 10_000_000 ether; + + // expresses the fee as a wad (i.e. 3000 = 0.003e18 = 0.30%) + uint256 FEE_WAD; + + LiquidityRange range; + + function setUp() public { + (alice, alicePK) = makeAddrAndKey("ALICE"); + (bob, bobPK) = makeAddrAndKey("BOB"); + + Deployers.deployFreshManagerAndRouters(); + Deployers.deployMintAndApprove2Currencies(); + + (key, poolId) = initPool(currency0, currency1, IHooks(address(0)), 3000, SQRT_PRICE_1_1, ZERO_BYTES); + FEE_WAD = uint256(key.fee).mulDivDown(FixedPointMathLib.WAD, 1_000_000); + + lpm = new NonfungiblePositionManager(manager); + IERC20(Currency.unwrap(currency0)).approve(address(lpm), type(uint256).max); + IERC20(Currency.unwrap(currency1)).approve(address(lpm), type(uint256).max); + + // Give tokens to Alice and Bob, with approvals + IERC20(Currency.unwrap(currency0)).transfer(alice, STARTING_USER_BALANCE); + IERC20(Currency.unwrap(currency1)).transfer(alice, STARTING_USER_BALANCE); + IERC20(Currency.unwrap(currency0)).transfer(bob, STARTING_USER_BALANCE); + IERC20(Currency.unwrap(currency1)).transfer(bob, STARTING_USER_BALANCE); + vm.startPrank(alice); + IERC20(Currency.unwrap(currency0)).approve(address(lpm), type(uint256).max); + IERC20(Currency.unwrap(currency1)).approve(address(lpm), type(uint256).max); + vm.stopPrank(); + vm.startPrank(bob); + IERC20(Currency.unwrap(currency0)).approve(address(lpm), type(uint256).max); + IERC20(Currency.unwrap(currency1)).approve(address(lpm), type(uint256).max); + vm.stopPrank(); + + // define a reusable range + range = LiquidityRange({poolKey: key, tickLower: -300, tickUpper: 300}); + } + + function test_permit_increaseLiquidity() public { + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // alice gives bob operator permissions + _permit(alice, alicePK, tokenIdAlice, bob); + + // bob can increase liquidity on alice's token + uint256 newLiquidity = 2e18; + uint256 balance0BobBefore = currency0.balanceOf(bob); + uint256 balance1BobBefore = currency1.balanceOf(bob); + vm.prank(bob); + _increaseLiquidity(tokenIdAlice, newLiquidity, ZERO_BYTES, false); + + // alice's position has new liquidity + (uint256 liquidity,,,,) = lpm.positions(alice, range.toId()); + assertEq(liquidity, liquidityAlice + newLiquidity); + + // bob used his tokens to increase liquidity + // TODO: enable after we fix msg.sender + // assertGt(balance0BobBefore, currency0.balanceOf(bob)); + // assertGt(balance1BobBefore, currency1.balanceOf(bob)); + } + + function test_permit_decreaseLiquidity() public { + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // alice gives bob operator permissions + _permit(alice, alicePK, tokenIdAlice, bob); + + // bob can decrease liquidity on alice's token + uint256 liquidityToRemove = 0.4444e18; + vm.prank(bob); + _decreaseLiquidity(tokenIdAlice, liquidityToRemove, ZERO_BYTES, false); + + // alice's position decreased liquidity + (uint256 liquidity,,,,) = lpm.positions(alice, range.toId()); + assertEq(liquidity, liquidityAlice - liquidityToRemove); + } + + function test_permit_collect() public { + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // donate to create fee revenue + uint256 currency0Revenue = 0.4444e18; + uint256 currency1Revenue = 0.2222e18; + donateRouter.donate(key, currency0Revenue, currency1Revenue, ZERO_BYTES); + + // alice gives bob operator permissions + _permit(alice, alicePK, tokenIdAlice, bob); + + // TODO: enable once we fix recipient collection + + // bob collects fees to a recipient + // address recipient = address(0x00444400); + // vm.startPrank(bob); + // _collect(tokenIdAlice, recipient, ZERO_BYTES, false); + // vm.stopPrank(); + + // assertEq(currency0.balanceOf(recipient), currency0Revenue); + // assertEq(currency1.balanceOf(recipient), currency1Revenue); + } + + // --- Fail Scenarios --- // + function test_permit_notOwnerRevert() public { + // calling permit on a token that is not owned will fail + + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // bob cannot permit himself on alice's token + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(bob), block.timestamp + 1); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(bobPK, digest); + + vm.startPrank(bob); + vm.expectRevert("Unauthorized"); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + vm.stopPrank(); + } + + function test_noPermit_increaseLiquidityRevert() public { + // increaseLiquidity fails if the owner did not permit + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // bob cannot increase liquidity on alice's token + uint256 newLiquidity = 2e18; + uint256 balance0BobBefore = currency0.balanceOf(bob); + uint256 balance1BobBefore = currency1.balanceOf(bob); + vm.startPrank(bob); + vm.expectRevert("Not approved"); + _increaseLiquidity(range, tokenIdAlice, newLiquidity, ZERO_BYTES, false); + vm.stopPrank(); + } + + function test_noPermit_decreaseLiquidityRevert() public { + // decreaseLiquidity fails if the owner did not permit + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // bob cannot decrease liquidity on alice's token + uint256 liquidityToRemove = 0.4444e18; + vm.startPrank(bob); + vm.expectRevert("Not approved"); + _decreaseLiquidity(range, tokenIdAlice, liquidityToRemove, ZERO_BYTES, false); + vm.stopPrank(); + } + + function test_noPermit_collectRevert() public { + // collect fails if the owner did not permit + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // donate to create fee revenue + uint256 currency0Revenue = 0.4444e18; + uint256 currency1Revenue = 0.2222e18; + donateRouter.donate(key, currency0Revenue, currency1Revenue, ZERO_BYTES); + + // bob cannot collect fees to a recipient + address recipient = address(0x00444400); + vm.startPrank(bob); + vm.expectRevert("Not approved"); + _collect(range, tokenIdAlice, recipient, ZERO_BYTES, false); + vm.stopPrank(); + } +} diff --git a/test/shared/LiquidityOperations.sol b/test/shared/LiquidityOperations.sol index a3d556cd..e96a55fc 100644 --- a/test/shared/LiquidityOperations.sol +++ b/test/shared/LiquidityOperations.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; +import {Vm} from "forge-std/Vm.sol"; import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; @@ -8,6 +9,7 @@ import {NonfungiblePositionManager} from "../../contracts/NonfungiblePositionMan import {LiquidityRange} from "../../contracts/types/LiquidityRange.sol"; contract LiquidityOperations { + Vm internal constant _vm1 = Vm(address(uint160(uint256(keccak256("hevm cheat code"))))); NonfungiblePositionManager lpm; function _mint( @@ -30,7 +32,19 @@ contract LiquidityOperations { bytes[] memory calls = new bytes[](1); calls[0] = abi.encodeWithSelector(lpm.increaseLiquidity.selector, tokenId, liquidityToAdd, hookData, claims); - (, LiquidityRange memory _range) = lpm.tokenPositions(tokenId); + (, LiquidityRange memory _range,) = lpm.tokenPositions(tokenId); + return _increaseLiquidity(_range, tokenId, liquidityToAdd, hookData, claims); + } + + function _increaseLiquidity( + LiquidityRange memory _range, + uint256 tokenId, + uint256 liquidityToAdd, + bytes memory hookData, + bool claims + ) internal { + bytes[] memory calls = new bytes[](1); + calls[0] = abi.encodeWithSelector(lpm.increaseLiquidity.selector, tokenId, liquidityToAdd, hookData, claims); Currency[] memory currencies = new Currency[](2); currencies[0] = _range.poolKey.currency0; @@ -42,14 +56,26 @@ contract LiquidityOperations { internal returns (BalanceDelta) { + (, LiquidityRange memory _range,) = lpm.tokenPositions(tokenId); + + return _decreaseLiquidity(_range, tokenId, liquidityToRemove, hookData, claims); + } + + // do not make external call before unlockAndExecute, allows us to test reverts + function _decreaseLiquidity( + LiquidityRange memory _range, + uint256 tokenId, + uint256 liquidityToRemove, + bytes memory hookData, + bool claims + ) internal returns (BalanceDelta) { bytes[] memory calls = new bytes[](1); calls[0] = abi.encodeWithSelector(lpm.decreaseLiquidity.selector, tokenId, liquidityToRemove, hookData, claims); - (, LiquidityRange memory _range) = lpm.tokenPositions(tokenId); - Currency[] memory currencies = new Currency[](2); currencies[0] = _range.poolKey.currency0; currencies[1] = _range.poolKey.currency1; + int128[] memory result = lpm.unlockAndExecute(calls, currencies); return toBalanceDelta(result[0], result[1]); } @@ -58,15 +84,36 @@ contract LiquidityOperations { internal returns (BalanceDelta) { + (, LiquidityRange memory _range,) = lpm.tokenPositions(tokenId); + return _collect(_range, tokenId, recipient, hookData, claims); + } + + // do not make external call before unlockAndExecute, allows us to test reverts + function _collect( + LiquidityRange memory _range, + uint256 tokenId, + address recipient, + bytes memory hookData, + bool claims + ) internal returns (BalanceDelta) { bytes[] memory calls = new bytes[](1); calls[0] = abi.encodeWithSelector(lpm.collect.selector, tokenId, recipient, hookData, claims); - (, LiquidityRange memory _range) = lpm.tokenPositions(tokenId); - Currency[] memory currencies = new Currency[](2); currencies[0] = _range.poolKey.currency0; currencies[1] = _range.poolKey.currency1; + int128[] memory result = lpm.unlockAndExecute(calls, currencies); return toBalanceDelta(result[0], result[1]); } + + // TODO: organize somewhere else, or rename this file to NFTLiquidityHelpers? + function _permit(address signer, uint256 privateKey, uint256 tokenId, address operator) internal { + bytes32 digest = lpm.getDigest(operator, tokenId, lpm.nonce(signer), block.timestamp + 1); + + (uint8 v, bytes32 r, bytes32 s) = _vm1.sign(privateKey, digest); + + _vm1.prank(signer); + lpm.permit(operator, tokenId, block.timestamp + 1, v, r, s); + } } From 07223775e15262e805047b73187f36d5e9420eb7 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sat, 6 Jul 2024 21:38:05 -0400 Subject: [PATCH 10/14] snapshots --- .forge-snapshots/autocompound_exactUnclaimedFees.snap | 2 +- .../autocompound_exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .forge-snapshots/autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc20.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .forge-snapshots/increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 30ef564b..6bc6bb71 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -293482 \ No newline at end of file +295633 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index 8715f5c7..ea6837fa 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -225841 \ No newline at end of file +227992 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index 87b78db1..d534acb8 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -314021 \ No newline at end of file +316172 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index 42c0f57b..cea64f5a 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1 +1 @@ -208541 \ No newline at end of file +210674 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index 786e11f0..f13370f5 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1 +1 @@ -208553 \ No newline at end of file +210686 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index f872a578..f38310d4 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -194298 \ No newline at end of file +196449 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index d9d4f89f..12be84a1 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -194310 \ No newline at end of file +196461 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index 39b5a098..10f5e7bb 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -513250 \ No newline at end of file +510048 \ No newline at end of file From 190adfc7db70403f5851f82c9f93431d0a5f4be2 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sat, 6 Jul 2024 21:55:17 -0400 Subject: [PATCH 11/14] gas benchmarks for permit --- .forge-snapshots/permit.snap | 1 + .forge-snapshots/permit_secondPosition.snap | 1 + .forge-snapshots/permit_twice.snap | 1 + test/position-managers/Gas.t.sol | 79 ++++++++++++++++++++- 4 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 .forge-snapshots/permit.snap create mode 100644 .forge-snapshots/permit_secondPosition.snap create mode 100644 .forge-snapshots/permit_twice.snap diff --git a/.forge-snapshots/permit.snap b/.forge-snapshots/permit.snap new file mode 100644 index 00000000..9927819f --- /dev/null +++ b/.forge-snapshots/permit.snap @@ -0,0 +1 @@ +74708 \ No newline at end of file diff --git a/.forge-snapshots/permit_secondPosition.snap b/.forge-snapshots/permit_secondPosition.snap new file mode 100644 index 00000000..9281d22b --- /dev/null +++ b/.forge-snapshots/permit_secondPosition.snap @@ -0,0 +1 @@ +57608 \ No newline at end of file diff --git a/.forge-snapshots/permit_twice.snap b/.forge-snapshots/permit_twice.snap new file mode 100644 index 00000000..6e951055 --- /dev/null +++ b/.forge-snapshots/permit_twice.snap @@ -0,0 +1 @@ +40496 \ No newline at end of file diff --git a/test/position-managers/Gas.t.sol b/test/position-managers/Gas.t.sol index 3d87fd77..54e23f56 100644 --- a/test/position-managers/Gas.t.sol +++ b/test/position-managers/Gas.t.sol @@ -32,8 +32,10 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { using PoolIdLibrary for PoolKey; PoolId poolId; - address alice = makeAddr("ALICE"); - address bob = makeAddr("BOB"); + address alice; + uint256 alicePK; + address bob; + uint256 bobPK; uint256 constant STARTING_USER_BALANCE = 10_000_000 ether; @@ -43,6 +45,9 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { LiquidityRange range; function setUp() public { + (alice, alicePK) = makeAddrAndKey("ALICE"); + (bob, bobPK) = makeAddrAndKey("BOB"); + Deployers.deployFreshManagerAndRouters(); Deployers.deployMintAndApprove2Currencies(); @@ -310,4 +315,74 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { function test_gas_burn() public {} function test_gas_burnEmpty() public {} function test_gas_collect() public {} + + function test_gas_permit() public { + // alice permits for the first time + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // alice gives operator permission to bob + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); + + vm.prank(alice); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + snapLastCall("permit"); + } + + function test_gas_permit_secondPosition() public { + // alice permits for her two tokens, benchmark the 2nd permit + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // alice gives operator permission to bob + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); + + vm.prank(alice); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + + // alice creates another position + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + tokenIdAlice = lpm.nextTokenId() - 1; + + // alice gives operator permission to bob + digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + (v, r, s) = vm.sign(alicePK, digest); + + vm.prank(alice); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + snapLastCall("permit_secondPosition"); + } + + function test_gas_permit_twice() public { + // alice permits the same token, twice + address charlie = makeAddr("CHARLIE"); + + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // alice gives operator permission to bob + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); + + vm.prank(alice); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + + // alice gives operator permission to charlie + digest = lpm.getDigest(charlie, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + (v, r, s) = vm.sign(alicePK, digest); + + vm.prank(alice); + lpm.permit(charlie, tokenIdAlice, block.timestamp + 1, v, r, s); + snapLastCall("permit_twice"); + } } From 8f50bdccd8489252b385cc865b1c5f2aeba2e948 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Wed, 10 Jul 2024 15:47:49 +0200 Subject: [PATCH 12/14] test fixes --- .../autocompound_exactUnclaimedFees.snap | 2 +- ...exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .../autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc20.snap | 2 +- .../decreaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .../increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- contracts/NonfungiblePositionManager.sol | 19 +++++++++++++++---- test/position-managers/Permit.t.sol | 9 ++++----- 10 files changed, 27 insertions(+), 17 deletions(-) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 8a26ccb9..b1bc5219 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -295465 \ No newline at end of file +295470 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index f1a243d3..b0cc5d95 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -227824 \ No newline at end of file +227829 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index 4458142e..455648f4 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -316004 \ No newline at end of file +316009 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index 9dce8ecd..7f0c9f02 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1 +1 @@ -213445 \ No newline at end of file +213449 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index 976b2131..9d6d2f0b 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1 +1 @@ -213455 \ No newline at end of file +213459 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index 6c047d59..7ed8a8ea 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -199081 \ No newline at end of file +199086 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index 00dd4bae..9f04cc1a 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -199093 \ No newline at end of file +199098 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index 34b9cf02..ebad633c 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -490191 \ No newline at end of file +490196 \ No newline at end of file diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index 5c5d4395..cc6026e9 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -20,6 +20,8 @@ import {TransientStateLibrary} from "@uniswap/v4-core/src/libraries/TransientSta import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; import {TransientLiquidityDelta} from "./libraries/TransientLiquidityDelta.sol"; +import "forge-std/console2.sol"; + contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidityManagement, ERC721Permit { using CurrencyLibrary for Currency; using CurrencySettleTake for Currency; @@ -62,11 +64,17 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit (bytes[] memory data, Currency[] memory currencies) = abi.decode(payload, (bytes[], Currency[])); bool success; - + bytes memory callReturn; for (uint256 i; i < data.length; i++) { // TODO: Move to internal call and bubble up all call return data. - (success,) = address(this).call(data[i]); - if (!success) revert("EXECUTE_FAILED"); + (success, callReturn) = address(this).call(data[i]); + if (!success) { + // if the call failed, bubble up the reason + /// @solidity memory-safe-assembly + assembly { + revert(add(callReturn, 32), mload(callReturn)) + } + } } // close the final deltas @@ -123,7 +131,10 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit } // TODO: in v3, we can partially collect fees, but what was the usecase here? - function collect(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) external { + function collect(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) + external + isAuthorizedForToken(tokenId) + { TokenPosition memory tokenPos = tokenPositions[tokenId]; _collect(recipient, tokenPos.owner, tokenPos.range, hookData); diff --git a/test/position-managers/Permit.t.sol b/test/position-managers/Permit.t.sol index 51295af5..e7939896 100644 --- a/test/position-managers/Permit.t.sol +++ b/test/position-managers/Permit.t.sol @@ -92,16 +92,15 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation uint256 balance0BobBefore = currency0.balanceOf(bob); uint256 balance1BobBefore = currency1.balanceOf(bob); vm.prank(bob); - _increaseLiquidity(tokenIdAlice, newLiquidity, ZERO_BYTES, false); + _increaseLiquidity(range, tokenIdAlice, newLiquidity, ZERO_BYTES, false); // alice's position has new liquidity (uint256 liquidity,,,,) = lpm.positions(alice, range.toId()); assertEq(liquidity, liquidityAlice + newLiquidity); // bob used his tokens to increase liquidity - // TODO: enable after we fix msg.sender - // assertGt(balance0BobBefore, currency0.balanceOf(bob)); - // assertGt(balance1BobBefore, currency1.balanceOf(bob)); + assertGt(balance0BobBefore, currency0.balanceOf(bob)); + assertGt(balance1BobBefore, currency1.balanceOf(bob)); } function test_permit_decreaseLiquidity() public { @@ -116,7 +115,7 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation // bob can decrease liquidity on alice's token uint256 liquidityToRemove = 0.4444e18; vm.prank(bob); - _decreaseLiquidity(tokenIdAlice, liquidityToRemove, ZERO_BYTES, false); + _decreaseLiquidity(range, tokenIdAlice, liquidityToRemove, ZERO_BYTES, false); // alice's position decreased liquidity (uint256 liquidity,,,,) = lpm.positions(alice, range.toId()); From ab7498250c5ade08e9ec1f3bced9d5b0200a31af Mon Sep 17 00:00:00 2001 From: saucepoint Date: Wed, 10 Jul 2024 16:57:31 +0200 Subject: [PATCH 13/14] unordered nonces --- .../autocompound_exactUnclaimedFees.snap | 2 +- ...exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .../autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .../increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/permit.snap | 2 +- .forge-snapshots/permit_secondPosition.snap | 2 +- .forge-snapshots/permit_twice.snap | 2 +- contracts/base/ERC721Permit.sol | 26 +++++++- contracts/interfaces/IERC721Permit.sol | 4 +- test/position-managers/Gas.t.sol | 25 ++++---- test/position-managers/Permit.t.sol | 59 +++++++++++++++++-- test/shared/LiquidityOperations.sol | 6 +- 13 files changed, 106 insertions(+), 30 deletions(-) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index b1bc5219..c71a0ae9 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -295470 \ No newline at end of file +295448 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index b0cc5d95..ddf3aa2d 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -227829 \ No newline at end of file +227807 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index 455648f4..058e94fb 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -316009 \ No newline at end of file +315987 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index 7ed8a8ea..afcbd9a4 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -199086 \ No newline at end of file +199064 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index 9f04cc1a..5009a3e7 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -199098 \ No newline at end of file +199076 \ No newline at end of file diff --git a/.forge-snapshots/permit.snap b/.forge-snapshots/permit.snap index 9927819f..269d80a6 100644 --- a/.forge-snapshots/permit.snap +++ b/.forge-snapshots/permit.snap @@ -1 +1 @@ -74708 \ No newline at end of file +75071 \ No newline at end of file diff --git a/.forge-snapshots/permit_secondPosition.snap b/.forge-snapshots/permit_secondPosition.snap index 9281d22b..d863e28d 100644 --- a/.forge-snapshots/permit_secondPosition.snap +++ b/.forge-snapshots/permit_secondPosition.snap @@ -1 +1 @@ -57608 \ No newline at end of file +57959 \ No newline at end of file diff --git a/.forge-snapshots/permit_twice.snap b/.forge-snapshots/permit_twice.snap index 6e951055..6ef9d761 100644 --- a/.forge-snapshots/permit_twice.snap +++ b/.forge-snapshots/permit_twice.snap @@ -1 +1 @@ -40496 \ No newline at end of file +40871 \ No newline at end of file diff --git a/contracts/base/ERC721Permit.sol b/contracts/base/ERC721Permit.sol index 8323c77e..4668f2c5 100644 --- a/contracts/base/ERC721Permit.sol +++ b/contracts/base/ERC721Permit.sol @@ -11,7 +11,7 @@ import {IERC1271} from "../interfaces/external/IERC1271.sol"; /// @title ERC721 with permit /// @notice Nonfungible tokens that support an approve via signature, i.e. permit abstract contract ERC721Permit is ERC721, IERC721Permit { - mapping(address owner => uint256 nonce) public nonce; + mapping(address owner => mapping(uint256 word => uint256 bitmap)) public nonces; /// @dev The hash of the name used in the permit signature verification bytes32 private immutable nameHash; @@ -45,7 +45,7 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { 0x49ecf333e5b8c95c40fdafc95c1ad136e8914a8fb55e9dc8bb01eaa83a2df9ad; /// @inheritdoc IERC721Permit - function permit(address spender, uint256 tokenId, uint256 deadline, uint8 v, bytes32 r, bytes32 s) + function permit(address spender, uint256 tokenId, uint256 deadline, uint256 nonce, uint8 v, bytes32 r, bytes32 s) external payable override @@ -55,7 +55,7 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { address owner = ownerOf(tokenId); require(spender != owner, "ERC721Permit: approval to current owner"); - bytes32 digest = getDigest(spender, tokenId, nonce[owner]++, deadline); + bytes32 digest = getDigest(spender, tokenId, nonce, deadline); if (Address.isContract(owner)) { require(IERC1271(owner).isValidSignature(digest, abi.encodePacked(r, s, v)) == 0x1626ba7e, "Unauthorized"); @@ -65,6 +65,7 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { require(recoveredAddress == owner, "Unauthorized"); } + _useUnorderedNonce(owner, nonce); approve(spender, tokenId); } @@ -81,4 +82,23 @@ abstract contract ERC721Permit is ERC721, IERC721Permit { ) ); } + + /// @notice Returns the index of the bitmap and the bit position within the bitmap. Used for unordered nonces + /// @param nonce The nonce to get the associated word and bit positions + /// @return wordPos The word position or index into the nonceBitmap + /// @return bitPos The bit position + /// @dev The first 248 bits of the nonce value is the index of the desired bitmap + /// @dev The last 8 bits of the nonce value is the position of the bit in the bitmap + function bitmapPositions(uint256 nonce) private pure returns (uint256 wordPos, uint256 bitPos) { + wordPos = uint248(nonce >> 8); + bitPos = uint8(nonce); + } + + function _useUnorderedNonce(address from, uint256 nonce) internal { + (uint256 wordPos, uint256 bitPos) = bitmapPositions(nonce); + uint256 bit = 1 << bitPos; + uint256 flipped = nonces[from][wordPos] ^= bit; + + if (flipped & bit == 0) revert NonceAlreadyUsed(); + } } diff --git a/contracts/interfaces/IERC721Permit.sol b/contracts/interfaces/IERC721Permit.sol index daa27030..213bca2a 100644 --- a/contracts/interfaces/IERC721Permit.sol +++ b/contracts/interfaces/IERC721Permit.sol @@ -4,6 +4,8 @@ pragma solidity >=0.7.5; /// @title ERC721 with permit /// @notice Extension to ERC721 that includes a permit function for signature based approvals interface IERC721Permit { + error NonceAlreadyUsed(); + /// @notice The permit typehash used in the permit signature /// @return The typehash for the permit function PERMIT_TYPEHASH() external pure returns (bytes32); @@ -19,7 +21,7 @@ interface IERC721Permit { /// @param v Must produce valid secp256k1 signature from the holder along with `r` and `s` /// @param r Must produce valid secp256k1 signature from the holder along with `v` and `s` /// @param s Must produce valid secp256k1 signature from the holder along with `r` and `v` - function permit(address spender, uint256 tokenId, uint256 deadline, uint8 v, bytes32 r, bytes32 s) + function permit(address spender, uint256 tokenId, uint256 deadline, uint256 nonce, uint8 v, bytes32 r, bytes32 s) external payable; } diff --git a/test/position-managers/Gas.t.sol b/test/position-managers/Gas.t.sol index 928ad3a2..23b81f39 100644 --- a/test/position-managers/Gas.t.sol +++ b/test/position-managers/Gas.t.sol @@ -324,12 +324,13 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { uint256 tokenIdAlice = lpm.nextTokenId() - 1; // alice gives operator permission to bob - bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + uint256 nonce = 1; + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); vm.prank(alice); - lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); snapLastCall("permit"); } @@ -341,11 +342,12 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { uint256 tokenIdAlice = lpm.nextTokenId() - 1; // alice gives operator permission to bob - bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + uint256 nonce = 1; + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); vm.prank(alice); - lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); // alice creates another position vm.prank(alice); @@ -353,11 +355,12 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { tokenIdAlice = lpm.nextTokenId() - 1; // alice gives operator permission to bob - digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + nonce = 2; + digest = lpm.getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); (v, r, s) = vm.sign(alicePK, digest); vm.prank(alice); - lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); snapLastCall("permit_secondPosition"); } @@ -371,18 +374,20 @@ contract GasTest is Test, Deployers, GasSnapshot, LiquidityOperations { uint256 tokenIdAlice = lpm.nextTokenId() - 1; // alice gives operator permission to bob - bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + uint256 nonce = 1; + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); vm.prank(alice); - lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); // alice gives operator permission to charlie - digest = lpm.getDigest(charlie, tokenIdAlice, lpm.nonce(alice), block.timestamp + 1); + nonce = 2; + digest = lpm.getDigest(charlie, tokenIdAlice, nonce, block.timestamp + 1); (v, r, s) = vm.sign(alicePK, digest); vm.prank(alice); - lpm.permit(charlie, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(charlie, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); snapLastCall("permit_twice"); } } diff --git a/test/position-managers/Permit.t.sol b/test/position-managers/Permit.t.sol index e7939896..1075ebcb 100644 --- a/test/position-managers/Permit.t.sol +++ b/test/position-managers/Permit.t.sol @@ -19,6 +19,7 @@ import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol"; import {IERC20} from "forge-std/interfaces/IERC20.sol"; import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; +import {IERC721Permit} from "../../contracts/interfaces/IERC721Permit.sol"; import {NonfungiblePositionManager} from "../../contracts/NonfungiblePositionManager.sol"; import {LiquidityRange, LiquidityRangeId, LiquidityRangeIdLibrary} from "../../contracts/types/LiquidityRange.sol"; @@ -85,7 +86,7 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation uint256 tokenIdAlice = lpm.nextTokenId() - 1; // alice gives bob operator permissions - _permit(alice, alicePK, tokenIdAlice, bob); + _permit(alice, alicePK, tokenIdAlice, bob, 1); // bob can increase liquidity on alice's token uint256 newLiquidity = 2e18; @@ -110,7 +111,7 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation uint256 tokenIdAlice = lpm.nextTokenId() - 1; // alice gives bob operator permissions - _permit(alice, alicePK, tokenIdAlice, bob); + _permit(alice, alicePK, tokenIdAlice, bob, 1); // bob can decrease liquidity on alice's token uint256 liquidityToRemove = 0.4444e18; @@ -134,7 +135,7 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation donateRouter.donate(key, currency0Revenue, currency1Revenue, ZERO_BYTES); // alice gives bob operator permissions - _permit(alice, alicePK, tokenIdAlice, bob); + _permit(alice, alicePK, tokenIdAlice, bob, 1); // TODO: enable once we fix recipient collection @@ -158,13 +159,13 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation uint256 tokenIdAlice = lpm.nextTokenId() - 1; // bob cannot permit himself on alice's token - bytes32 digest = lpm.getDigest(bob, tokenIdAlice, lpm.nonce(bob), block.timestamp + 1); + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, 0, block.timestamp + 1); (uint8 v, bytes32 r, bytes32 s) = vm.sign(bobPK, digest); vm.startPrank(bob); vm.expectRevert("Unauthorized"); - lpm.permit(bob, tokenIdAlice, block.timestamp + 1, v, r, s); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, 0, v, r, s); vm.stopPrank(); } @@ -219,4 +220,52 @@ contract PermitTest is Test, Deployers, GasSnapshot, Fuzzers, LiquidityOperation _collect(range, tokenIdAlice, recipient, ZERO_BYTES, false); vm.stopPrank(); } + + function test_permit_nonceAlreadyUsed() public { + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // alice gives bob operator permissions + uint256 nonce = 1; + _permit(alice, alicePK, tokenIdAlice, bob, nonce); + + // alice cannot reuse the nonce + bytes32 digest = lpm.getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); + + vm.startPrank(alice); + vm.expectRevert(IERC721Permit.NonceAlreadyUsed.selector); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, v, r, s); + vm.stopPrank(); + } + + function test_permit_nonceAlreadyUsed_twoPositions() public { + uint256 liquidityAlice = 1e18; + vm.prank(alice); + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + vm.prank(alice); + range.tickLower = -600; + range.tickUpper = 600; + _mint(range, liquidityAlice, block.timestamp + 1, alice, ZERO_BYTES); + uint256 tokenIdAlice2 = lpm.nextTokenId() - 1; + + // alice gives bob operator permissions for first token + uint256 nonce = 1; + _permit(alice, alicePK, tokenIdAlice, bob, nonce); + + // alice cannot reuse the nonce for the second token + bytes32 digest = lpm.getDigest(bob, tokenIdAlice2, nonce, block.timestamp + 1); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); + + vm.startPrank(alice); + vm.expectRevert(IERC721Permit.NonceAlreadyUsed.selector); + lpm.permit(bob, tokenIdAlice2, block.timestamp + 1, nonce, v, r, s); + vm.stopPrank(); + } } diff --git a/test/shared/LiquidityOperations.sol b/test/shared/LiquidityOperations.sol index 5b01b1f7..def18fde 100644 --- a/test/shared/LiquidityOperations.sol +++ b/test/shared/LiquidityOperations.sol @@ -108,12 +108,12 @@ contract LiquidityOperations { } // TODO: organize somewhere else, or rename this file to NFTLiquidityHelpers? - function _permit(address signer, uint256 privateKey, uint256 tokenId, address operator) internal { - bytes32 digest = lpm.getDigest(operator, tokenId, lpm.nonce(signer), block.timestamp + 1); + function _permit(address signer, uint256 privateKey, uint256 tokenId, address operator, uint256 nonce) internal { + bytes32 digest = lpm.getDigest(operator, tokenId, 1, block.timestamp + 1); (uint8 v, bytes32 r, bytes32 s) = _vm1.sign(privateKey, digest); _vm1.prank(signer); - lpm.permit(operator, tokenId, block.timestamp + 1, v, r, s); + lpm.permit(operator, tokenId, block.timestamp + 1, nonce, v, r, s); } } From dc5d4d4f2253f3fa794d0aa52d3372f32d3da641 Mon Sep 17 00:00:00 2001 From: saucepoint Date: Mon, 15 Jul 2024 11:35:08 +0200 Subject: [PATCH 14/14] fix tests / cheatcode usage --- test/shared/LiquidityOperations.sol | 30 +++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/test/shared/LiquidityOperations.sol b/test/shared/LiquidityOperations.sol index bf07ad4c..7b9fbdac 100644 --- a/test/shared/LiquidityOperations.sol +++ b/test/shared/LiquidityOperations.sol @@ -32,7 +32,10 @@ contract LiquidityOperations { return abi.decode(result[0], (BalanceDelta)); } - function _increaseLiquidity(uint256 tokenId, uint256 liquidityToAdd, bytes memory hookData, bool claims) internal { + function _increaseLiquidity(uint256 tokenId, uint256 liquidityToAdd, bytes memory hookData, bool claims) + internal + returns (BalanceDelta) + { (, LiquidityRange memory _range,) = lpm.tokenPositions(tokenId); return _increaseLiquidity(_range, tokenId, liquidityToAdd, hookData, claims); } @@ -43,14 +46,18 @@ contract LiquidityOperations { uint256 liquidityToAdd, bytes memory hookData, bool claims - ) internal { - Planner.Plan memory planner = Planner.init(); - planner = planner.add(Actions.INCREASE, abi.encode(tokenId, liquidityToAdd, hookData, claims)); + ) internal returns (BalanceDelta) { + // cannot use Planner because it interferes with cheatcodes + Actions[] memory actions = new Actions[](1); + actions[0] = Actions.INCREASE; + bytes[] memory params = new bytes[](1); + params[0] = abi.encode(tokenId, liquidityToAdd, hookData, claims); Currency[] memory currencies = new Currency[](2); currencies[0] = _range.poolKey.currency0; currencies[1] = _range.poolKey.currency1; - lpm.modifyLiquidities(abi.encode(planner.actions, planner.params, currencies)); + bytes[] memory result = lpm.modifyLiquidities(abi.encode(actions, params, currencies)); + if (result.length > 0) return abi.decode(result[0], (BalanceDelta)); } function _decreaseLiquidity(uint256 tokenId, uint256 liquidityToRemove, bytes memory hookData, bool claims) @@ -80,7 +87,7 @@ contract LiquidityOperations { currencies[0] = _range.poolKey.currency0; currencies[1] = _range.poolKey.currency1; bytes[] memory result = lpm.modifyLiquidities(abi.encode(actions, params, currencies)); - return abi.decode(result[0], (BalanceDelta)); + if (result.length > 0) return abi.decode(result[0], (BalanceDelta)); } function _collect(uint256 tokenId, address recipient, bytes memory hookData, bool claims) @@ -99,14 +106,17 @@ contract LiquidityOperations { bytes memory hookData, bool claims ) internal returns (BalanceDelta) { - Planner.Plan memory planner = Planner.init(); - planner = planner.add(Actions.COLLECT, abi.encode(tokenId, recipient, hookData, claims)); + // cannot use Planner because it interferes with cheatcodes + Actions[] memory actions = new Actions[](1); + actions[0] = Actions.COLLECT; + bytes[] memory params = new bytes[](1); + params[0] = abi.encode(tokenId, recipient, hookData, claims); Currency[] memory currencies = new Currency[](2); currencies[0] = _range.poolKey.currency0; currencies[1] = _range.poolKey.currency1; - bytes[] memory result = lpm.modifyLiquidities(abi.encode(planner.actions, planner.params, currencies)); - return abi.decode(result[0], (BalanceDelta)); + bytes[] memory result = lpm.modifyLiquidities(abi.encode(actions, params, currencies)); + if (result.length > 0) return abi.decode(result[0], (BalanceDelta)); } function _burn(uint256 tokenId) internal {