Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flip the sign of deltas #481

Merged
merged 31 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
d93b3c9
Remove access lock
hensha256 Dec 11, 2023
13642d8
remove current hook
hensha256 Dec 11, 2023
cd92291
Remove nested locking
hensha256 Dec 11, 2023
1f4961e
Locker library tests
hensha256 Dec 12, 2023
c0e2722
Rename empty lock test
hensha256 Dec 12, 2023
2237a48
Test skeleton
hensha256 Dec 12, 2023
df58a45
Test for a nested swap
hensha256 Dec 13, 2023
99243a1
Tests for nested function calls
hensha256 Dec 14, 2023
514726d
Merge branch 'main' into remove-nested-locks
hensha256 Dec 15, 2023
9c8f549
Merge branch 'main' into remove-nested-locks
hensha256 Dec 15, 2023
250d74d
Merge branch 'main' into remove-nested-locks
hensha256 Dec 18, 2023
56f6044
snapshots
hensha256 Dec 18, 2023
6ce20c4
Separate libs, rename error
hensha256 Jan 31, 2024
96975e7
Merge branch 'main' into remove-nested-locks
hensha256 Feb 12, 2024
115341e
Merge branch 'main' into remove-nested-locks
hensha256 Feb 12, 2024
c802859
Remove lock caller
hensha256 Feb 12, 2024
67f79e8
Merge main into remove-nested-locks
hensha256 Feb 14, 2024
dff0d82
merge errors
hensha256 Feb 14, 2024
47bea1a
Flipping the sign of deltas
hensha256 Feb 14, 2024
ba1494d
update comments
hensha256 Feb 14, 2024
0f19c28
Merge branch 'main' into flip-the-deltas
hensha256 Feb 29, 2024
7a5d41d
Merge branch 'main' into flip-the-deltas
hensha256 Mar 5, 2024
b746376
Merge branch 'main' into flip-the-deltas
hensha256 Mar 6, 2024
d8ebb60
remove unnecessary read of result
hensha256 Mar 6, 2024
f45c75a
move delta flip to sqrtprice lib
hensha256 Mar 6, 2024
0043fa4
remove duplicate test
hensha256 Mar 6, 2024
1a76db2
amountSpecified matches deltas (#491)
ewilz Mar 6, 2024
4a1ccfa
PR nits
hensha256 Mar 8, 2024
9fcf92f
Merge branch 'main' into flip-the-deltas
hensha256 Mar 8, 2024
b02bda8
Merge branch 'main' into flip-the-deltas
hensha256 Mar 13, 2024
8ffcc42
Merge branch 'main' into flip-the-deltas
hensha256 Mar 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/SwapMath_oneForZero_exactInCapped.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2203
1947
2 changes: 1 addition & 1 deletion .forge-snapshots/SwapMath_oneForZero_exactInPartial.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3028
3238
2 changes: 1 addition & 1 deletion .forge-snapshots/SwapMath_oneForZero_exactOutCapped.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1963
2208
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3028
3238
2 changes: 1 addition & 1 deletion .forge-snapshots/SwapMath_zeroForOne_exactInCapped.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2193
1937
2 changes: 1 addition & 1 deletion .forge-snapshots/SwapMath_zeroForOne_exactInPartial.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3181
2826
2 changes: 1 addition & 1 deletion .forge-snapshots/SwapMath_zeroForOne_exactOutCapped.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1953
2198
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3181
2826
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity with empty hook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
312661
312478
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity with native token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
192843
192657
2 changes: 1 addition & 1 deletion .forge-snapshots/addLiquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
192821
192638
Original file line number Diff line number Diff line change
@@ -1 +1 @@
184735
184850
2 changes: 1 addition & 1 deletion .forge-snapshots/cached dynamic fee, no hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
138762
138877
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 1 token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
131854
132225
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 2 tokens.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
176958
177402
1 change: 1 addition & 0 deletions .forge-snapshots/mintWithEmptyHookEOAInitiated.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
247213
1 change: 1 addition & 0 deletions .forge-snapshots/modify position with noop.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
54090
2 changes: 1 addition & 1 deletion .forge-snapshots/poolManager bytecode size.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
23345
23287
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity with empty hook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
99632
99211
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity with native token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
201005
200584
2 changes: 1 addition & 1 deletion .forge-snapshots/removeLiquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
197282
196861
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap with native.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
187335
187447
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
195872
195987
1 change: 1 addition & 0 deletions .forge-snapshots/simpleSwapEOAInitiated.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
166598
1 change: 1 addition & 0 deletions .forge-snapshots/simpleSwapNativeEOAInitiated.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
165126
Original file line number Diff line number Diff line change
@@ -1 +1 @@
117998
118106
2 changes: 1 addition & 1 deletion .forge-snapshots/swap against liquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
105462
105573
2 changes: 1 addition & 1 deletion .forge-snapshots/swap burn 6909 for input.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
125651
125700
2 changes: 1 addition & 1 deletion .forge-snapshots/swap burn native 6909 for input.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
121604
121653
2 changes: 1 addition & 1 deletion .forge-snapshots/swap mint native output as 6909.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
189861
190005
2 changes: 1 addition & 1 deletion .forge-snapshots/swap mint output as 6909.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
206666
206814
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with dynamic fee.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
183836
183951
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
105440
105551
2 changes: 1 addition & 1 deletion .forge-snapshots/update dynamic fee in before swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
190540
190655
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ A more detailed description of Uniswap v4 Core can be found in the draft of the
- `settle`
- `mint`

Only the net balances owed to the pool (positive) or to the user (negative) are tracked throughout the duration of a lock. This is the `delta` field held in the lock state. Any number of actions can be run on the pools, as long as the deltas accumulated during the lock reach 0 by the lock’s release. This lock and call style architecture gives callers maximum flexibility in integrating with the core code.
Only the net balances owed to the user (positive) or to the pool (negative) are tracked throughout the duration of a lock. This is the `delta` field held in the lock state. Any number of actions can be run on the pools, as long as the deltas accumulated during the lock reach 0 by the lock’s release. This lock and call style architecture gives callers maximum flexibility in integrating with the core code.

Additionally, a pool may be initialized with a hook contract, that can implement any of the following callbacks in the lifecycle of pool actions:

Expand Down
13 changes: 7 additions & 6 deletions src/PoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,8 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC6909Claims {
);

_accountPoolBalanceDelta(key, delta);
// the fee is on the input currency

// the fee is on the input currency
unchecked {
if (feeForProtocol > 0) {
protocolFeesAccrued[params.zeroForOne ? key.currency0 : key.currency1] += feeForProtocol;
Expand Down Expand Up @@ -258,7 +258,8 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC6909Claims {

/// @inheritdoc IPoolManager
function take(Currency currency, address to, uint256 amount) external override noDelegateCall isLocked {
_accountDelta(currency, amount.toInt128());
// subtraction must be safe
_accountDelta(currency, -(amount.toInt128()));
reservesOf[currency] -= amount;
currency.transfer(to, amount);
}
Expand All @@ -268,19 +269,19 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, ERC6909Claims {
uint256 reservesBefore = reservesOf[currency];
reservesOf[currency] = currency.balanceOfSelf();
paid = reservesOf[currency] - reservesBefore;
// subtraction must be safe
_accountDelta(currency, -(paid.toInt128()));
_accountDelta(currency, paid.toInt128());
}

/// @inheritdoc IPoolManager
function mint(address to, uint256 id, uint256 amount) external override noDelegateCall isLocked {
_accountDelta(CurrencyLibrary.fromId(id), amount.toInt128());
// subtraction must be safe
_accountDelta(CurrencyLibrary.fromId(id), -(amount.toInt128()));
_mint(to, id, amount);
}

/// @inheritdoc IPoolManager
function burn(address from, uint256 id, uint256 amount) external override noDelegateCall isLocked {
_accountDelta(CurrencyLibrary.fromId(id), -(amount.toInt128()));
_accountDelta(CurrencyLibrary.fromId(id), amount.toInt128());
_burnFrom(from, id, amount);
}

Expand Down
67 changes: 32 additions & 35 deletions src/libraries/Pool.sol
Original file line number Diff line number Diff line change
Expand Up @@ -209,46 +209,43 @@ library Pool {
if (self.slot0.tick < params.tickLower) {
// current tick is below the passed range; liquidity can only become in range by crossing from left to
// right, when we'll need _more_ currency0 (it's becoming more valuable) so user must provide it
result = result
+ toBalanceDelta(
SqrtPriceMath.getAmount0Delta(
TickMath.getSqrtRatioAtTick(params.tickLower),
TickMath.getSqrtRatioAtTick(params.tickUpper),
params.liquidityDelta
).toInt128(),
0
);
result = toBalanceDelta(
SqrtPriceMath.getAmount0Delta(
TickMath.getSqrtRatioAtTick(params.tickLower),
TickMath.getSqrtRatioAtTick(params.tickUpper),
params.liquidityDelta
).toInt128(),
0
);
} else if (self.slot0.tick < params.tickUpper) {
result = result
+ toBalanceDelta(
SqrtPriceMath.getAmount0Delta(
self.slot0.sqrtPriceX96, TickMath.getSqrtRatioAtTick(params.tickUpper), params.liquidityDelta
).toInt128(),
SqrtPriceMath.getAmount1Delta(
TickMath.getSqrtRatioAtTick(params.tickLower), self.slot0.sqrtPriceX96, params.liquidityDelta
).toInt128()
);
result = toBalanceDelta(
SqrtPriceMath.getAmount0Delta(
self.slot0.sqrtPriceX96, TickMath.getSqrtRatioAtTick(params.tickUpper), params.liquidityDelta
).toInt128(),
SqrtPriceMath.getAmount1Delta(
TickMath.getSqrtRatioAtTick(params.tickLower), self.slot0.sqrtPriceX96, params.liquidityDelta
).toInt128()
);

self.liquidity = params.liquidityDelta < 0
? self.liquidity - uint128(-params.liquidityDelta)
: self.liquidity + uint128(params.liquidityDelta);
} else {
// current tick is above the passed range; liquidity can only become in range by crossing from right to
// left, when we'll need _more_ currency1 (it's becoming more valuable) so user must provide it
result = result
+ toBalanceDelta(
0,
SqrtPriceMath.getAmount1Delta(
TickMath.getSqrtRatioAtTick(params.tickLower),
TickMath.getSqrtRatioAtTick(params.tickUpper),
params.liquidityDelta
).toInt128()
);
result = toBalanceDelta(
0,
SqrtPriceMath.getAmount1Delta(
TickMath.getSqrtRatioAtTick(params.tickLower),
TickMath.getSqrtRatioAtTick(params.tickUpper),
params.liquidityDelta
).toInt128()
);
}
}

// Fees earned from LPing are removed from the pool balance.
result = result - toBalanceDelta(feesOwed0.toInt128(), feesOwed1.toInt128());
// Fees earned from LPing are added to the user's currency delta.
result = result + toBalanceDelta(feesOwed0.toInt128(), feesOwed1.toInt128());
}

struct SwapCache {
Expand Down Expand Up @@ -329,7 +326,7 @@ library Pool {
protocolFee: params.zeroForOne ? uint8(slot0Start.protocolFee % 256) : uint8(slot0Start.protocolFee >> 8)
});

bool exactInput = params.amountSpecified > 0;
bool exactInput = params.amountSpecified < 0;

state = SwapState({
amountSpecifiedRemaining: params.amountSpecified,
Expand Down Expand Up @@ -374,14 +371,14 @@ library Pool {
if (exactInput) {
// safe because we test that amountSpecified > amountIn + feeAmount in SwapMath
unchecked {
state.amountSpecifiedRemaining -= (step.amountIn + step.feeAmount).toInt256();
state.amountSpecifiedRemaining += (step.amountIn + step.feeAmount).toInt256();
}
state.amountCalculated = state.amountCalculated - step.amountOut.toInt256();
state.amountCalculated = state.amountCalculated + step.amountOut.toInt256();
} else {
unchecked {
state.amountSpecifiedRemaining += step.amountOut.toInt256();
state.amountSpecifiedRemaining -= step.amountOut.toInt256();
}
state.amountCalculated = state.amountCalculated + (step.amountIn + step.feeAmount).toInt256();
state.amountCalculated = state.amountCalculated - (step.amountIn + step.feeAmount).toInt256();
}

// if the protocol fee is on, calculate how much is owed, decrement feeAmount, and increment protocolFee
Expand Down Expand Up @@ -462,7 +459,7 @@ library Pool {
/// @notice Donates the given amount of currency0 and currency1 to the pool
function donate(State storage state, uint256 amount0, uint256 amount1) internal returns (BalanceDelta delta) {
if (state.liquidity == 0) revert NoLiquidityToReceiveFees();
delta = toBalanceDelta(amount0.toInt128(), amount1.toInt128());
delta = toBalanceDelta(-(amount0.toInt128()), -(amount1.toInt128()));
unchecked {
if (amount0 > 0) {
state.feeGrowthGlobal0X128 += FullMath.mulDiv(amount0, FixedPoint128.Q128, state.liquidity);
Expand Down
8 changes: 4 additions & 4 deletions src/libraries/SqrtPriceMath.sol
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,8 @@ library SqrtPriceMath {
{
unchecked {
return liquidity < 0
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we don't want to switch liquidityDelta sign if we are changing amountSpecified sign? Now they don't match right?

IE liquidityDelta is positive, we return negative delta because user owes $ to pool and if liquidityDelta is negative (user is taking liquidity out of the pool) we return a positive delta because the user can now claim it back from the pool...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right the argument is that liquidityDelta is the user's delta of liquidity units not token units. When they pass in liquidityDelta = 100 its because they want their liquidity balance to increase by 100 (which means sending tokens into the contract). So liquidityDelta is in line with amountSpecified now... for both of them:

  • positive: the balance of the thing in question goes up
  • negative: the balance of the thing in question goes down

Copy link
Member

Choose a reason for hiding this comment

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

great, makes sense! i think im in agreement w that framing then

? -getAmount0Delta(sqrtRatioAX96, sqrtRatioBX96, uint128(-liquidity), false).toInt256()
: getAmount0Delta(sqrtRatioAX96, sqrtRatioBX96, uint128(liquidity), true).toInt256();
? getAmount0Delta(sqrtRatioAX96, sqrtRatioBX96, uint128(-liquidity), false).toInt256()
: -getAmount0Delta(sqrtRatioAX96, sqrtRatioBX96, uint128(liquidity), true).toInt256();
}
}

Expand All @@ -217,8 +217,8 @@ library SqrtPriceMath {
{
unchecked {
return liquidity < 0
? -getAmount1Delta(sqrtRatioAX96, sqrtRatioBX96, uint128(-liquidity), false).toInt256()
: getAmount1Delta(sqrtRatioAX96, sqrtRatioBX96, uint128(liquidity), true).toInt256();
? getAmount1Delta(sqrtRatioAX96, sqrtRatioBX96, uint128(-liquidity), false).toInt256()
: -getAmount1Delta(sqrtRatioAX96, sqrtRatioBX96, uint128(liquidity), true).toInt256();
}
}
}
14 changes: 7 additions & 7 deletions src/libraries/SwapMath.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ library SwapMath {
) internal pure returns (uint160 sqrtRatioNextX96, uint256 amountIn, uint256 amountOut, uint256 feeAmount) {
unchecked {
bool zeroForOne = sqrtRatioCurrentX96 >= sqrtRatioTargetX96;
bool exactIn = amountRemaining >= 0;
bool exactIn = amountRemaining < 0;

if (exactIn) {
uint256 amountRemainingLessFee = FullMath.mulDiv(uint256(amountRemaining), 1e6 - feePips, 1e6);
uint256 amountRemainingLessFee = FullMath.mulDiv(uint256(-amountRemaining), 1e6 - feePips, 1e6);
amountIn = zeroForOne
? SqrtPriceMath.getAmount0Delta(sqrtRatioTargetX96, sqrtRatioCurrentX96, liquidity, true)
: SqrtPriceMath.getAmount1Delta(sqrtRatioCurrentX96, sqrtRatioTargetX96, liquidity, true);
Expand All @@ -45,11 +45,11 @@ library SwapMath {
amountOut = zeroForOne
? SqrtPriceMath.getAmount1Delta(sqrtRatioTargetX96, sqrtRatioCurrentX96, liquidity, false)
: SqrtPriceMath.getAmount0Delta(sqrtRatioCurrentX96, sqrtRatioTargetX96, liquidity, false);
if (uint256(-amountRemaining) >= amountOut) {
if (uint256(amountRemaining) >= amountOut) {
sqrtRatioNextX96 = sqrtRatioTargetX96;
} else {
sqrtRatioNextX96 = SqrtPriceMath.getNextSqrtPriceFromOutput(
sqrtRatioCurrentX96, liquidity, uint256(-amountRemaining), zeroForOne
sqrtRatioCurrentX96, liquidity, uint256(amountRemaining), zeroForOne
);
}
}
Expand All @@ -74,13 +74,13 @@ library SwapMath {
}

// cap the output amount to not exceed the remaining output amount
if (!exactIn && amountOut > uint256(-amountRemaining)) {
amountOut = uint256(-amountRemaining);
if (!exactIn && amountOut > uint256(amountRemaining)) {
amountOut = uint256(amountRemaining);
}

if (exactIn && sqrtRatioNextX96 != sqrtRatioTargetX96) {
// we didn't reach the target, so take the remainder of the maximum input as fee
feeAmount = uint256(amountRemaining) - amountIn;
feeAmount = uint256(-amountRemaining) - amountIn;
Copy link
Member

Choose a reason for hiding this comment

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

I think I'm not 100% understanding this line. If we have't reached the target the feeAmount is the amount left to swap minus the amount already swapped?

Also why does this case only happen in the exactIn case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have't reached the target the feeAmount is the amount left to swap minus the amount already swapped?

If we havent reached the target (which is the essentially maximum slippage where the trade has to stop), that means that all of the amountRemaining has successfully be traded into output tokens. The case this happens, the function follows the following flow...

  • calculate amountRemainingLessFee which is just the amount to be traded, minus the LP fee
  • calculates the sqrtRatioNextX96 that does get reached by this amount of input tokens
  • calculate the exact amountIn needed to trade to that next sqrtRatio using getAmount0Delta. Due to rounding of calculations this could be slightly different to the amountRemainingLessFee that it was going to trade before
  • now the true LP fee amount needs to be calculated, which is the difference between the total amount to be traded amountRemaining (which is now negative so needs to be flipped), and amountIn which is the amount thats actually being swapped. This means that the fee is rounded in the LP's favour.

Also why does this case only happen in the exactIn case?

LP fees are taken on input tokens. For exactIn, the calculation makes sure that the final amountIn including fees is amountRemaining. For non-exactIn the fee amount can just be calculated as feePips% of the amountIn, again rounded in the LP's favour. Theres not a need to make sure that the total feeAmount + amountIn == amountRemaining because its not exactIn so the "exact" part stops mattering

} else {
feeAmount = FullMath.mulDivRoundingUp(amountIn, feePips, 1e6 - feePips);
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/PoolClaimsTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ contract PoolClaimsTest is PoolTestBase {

if (data.deposit) {
manager.mint(data.user, data.currency.toId(), uint128(data.amount));
_settle(data.currency, data.user, data.amount.toInt128(), true);
_settle(data.currency, data.user, -data.amount.toInt128(), true);
} else {
manager.burn(data.user, data.currency.toId(), uint128(data.amount));
_take(data.currency, data.user, data.amount.toInt128(), true);
Expand Down
Loading
Loading