Skip to content

Commit

Permalink
Burn accounting tokens owed (Uniswap#427)
Browse files Browse the repository at this point in the history 
* rename

* do not transfer in burn

* Update contracts/UniswapV3Pool.sol

Co-authored-by: Noah Zinsmeister <[email protected]>

* fix a test

* fixing tests

* fix more tests

* fix remaining tests

* fix last test

* one more test

* ok one more fix

* one more nit

Co-authored-by: Noah Zinsmeister <[email protected]>
  • Loading branch information
@moodysalem @NoahZinsmeister
moodysalem and NoahZinsmeister authored Mar 1, 2021
1 parent 5c5276d commit 5515da1
Show file tree
Hide file tree
Showing 12 changed files with 237 additions and 205 deletions.
44 changes: 29 additions & 15 deletions contracts/UniswapV3Pool.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,18 +223,29 @@ contract UniswapV3Pool is IUniswapV3Pool, NoDelegateCall {

/// @dev Effect some changes to a position
/// @param params the position details and the change to the position's liquidity to effect
/// @return position a storage pointer referencing the position with the given owner and tick range
/// @return amount0 the amount of token0 owed to the pool, negative if the pool should pay the recipient
/// @return amount1 the amount of token1 owed to the pool, negative if the pool should pay the recipient
function _modifyPosition(ModifyPositionParams memory params)
private
noDelegateCall
returns (int256 amount0, int256 amount1)
returns (
Position.Info storage position,
int256 amount0,
int256 amount1
)
{
checkTicks(params.tickLower, params.tickUpper);

Slot0 memory _slot0 = slot0; // SLOAD for gas optimization

_updatePosition(params.owner, params.tickLower, params.tickUpper, params.liquidityDelta, _slot0.tick);
position = _updatePosition(
params.owner,
params.tickLower,
params.tickUpper,
params.liquidityDelta,
_slot0.tick
);

if (params.liquidityDelta != 0) {
if (_slot0.tick < params.tickLower) {
Expand Down Expand Up @@ -294,8 +305,8 @@ contract UniswapV3Pool is IUniswapV3Pool, NoDelegateCall {
int24 tickUpper,
int128 liquidityDelta,
int24 tick
) private {
Position.Info storage position = positions.get(owner, tickLower, tickUpper);
) private returns (Position.Info storage position) {
position = positions.get(owner, tickLower, tickUpper);

uint256 _feeGrowthGlobal0X128 = feeGrowthGlobal0X128; // SLOAD for gas optimization
uint256 _feeGrowthGlobal1X128 = feeGrowthGlobal1X128; // SLOAD for gas optimization
Expand Down Expand Up @@ -363,7 +374,7 @@ contract UniswapV3Pool is IUniswapV3Pool, NoDelegateCall {
bytes calldata data
) external override lock returns (uint256 amount0, uint256 amount1) {
require(amount > 0);
(int256 amount0Int, int256 amount1Int) =
(, int256 amount0Int, int256 amount1Int) =
_modifyPosition(
ModifyPositionParams({
owner: recipient,
Expand Down Expand Up @@ -395,18 +406,18 @@ contract UniswapV3Pool is IUniswapV3Pool, NoDelegateCall {
uint128 amount0Requested,
uint128 amount1Requested
) external override lock returns (uint128 amount0, uint128 amount1) {
// we don't need to checkTicks here, because invalid positions will never have non-zero feesOwed{0,1}
// we don't need to checkTicks here, because invalid positions will never have non-zero tokensOwed{0,1}
Position.Info storage position = positions.get(msg.sender, tickLower, tickUpper);

amount0 = amount0Requested > position.feesOwed0 ? position.feesOwed0 : amount0Requested;
amount1 = amount1Requested > position.feesOwed1 ? position.feesOwed1 : amount1Requested;
amount0 = amount0Requested > position.tokensOwed0 ? position.tokensOwed0 : amount0Requested;
amount1 = amount1Requested > position.tokensOwed1 ? position.tokensOwed1 : amount1Requested;

if (amount0 > 0) {
position.feesOwed0 -= amount0;
position.tokensOwed0 -= amount0;
TransferHelper.safeTransfer(token0, recipient, amount0);
}
if (amount1 > 0) {
position.feesOwed1 -= amount1;
position.tokensOwed1 -= amount1;
TransferHelper.safeTransfer(token1, recipient, amount1);
}

Expand All @@ -416,12 +427,11 @@ contract UniswapV3Pool is IUniswapV3Pool, NoDelegateCall {
/// @inheritdoc IUniswapV3PoolActions
/// @dev noDelegateCall is applied indirectly via _modifyPosition
function burn(
address recipient,
int24 tickLower,
int24 tickUpper,
uint128 amount
) external override lock returns (uint256 amount0, uint256 amount1) {
(int256 amount0Int, int256 amount1Int) =
(Position.Info storage position, int256 amount0Int, int256 amount1Int) =
_modifyPosition(
ModifyPositionParams({
owner: msg.sender,
Expand All @@ -434,10 +444,14 @@ contract UniswapV3Pool is IUniswapV3Pool, NoDelegateCall {
amount0 = uint256(-amount0Int);
amount1 = uint256(-amount1Int);

if (amount0 > 0) TransferHelper.safeTransfer(token0, recipient, amount0);
if (amount1 > 0) TransferHelper.safeTransfer(token1, recipient, amount1);
if (amount0 > 0 || amount1 > 0) {
(position.tokensOwed0, position.tokensOwed1) = (
position.tokensOwed0 + uint128(amount0),
position.tokensOwed1 + uint128(amount1)
);
}

emit Burn(msg.sender, recipient, tickLower, tickUpper, amount, amount0, amount1);
emit Burn(msg.sender, tickLower, tickUpper, amount, amount0, amount1);
}

struct SwapCache {
Expand Down
4 changes: 1 addition & 3 deletions contracts/interfaces/pool/IUniswapV3PoolActions.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,15 @@ interface IUniswapV3PoolActions {
uint128 amount1Requested
) external returns (uint128 amount0, uint128 amount1);

/// @notice Burn liquidity from the sender and send any tokens owed for the liquidity to a recipient
/// @notice Burn liquidity from the sender and account tokens owed for the liquidity to the position
/// @dev Can be used to trigger a recalculation of fees owed to a position by calling with an amount of 0
/// @dev Fees must be collected separately via a call to #collect
/// @param recipient The address which should receive the tokens
/// @param tickLower The lower tick of the position for which to burn liquidity
/// @param tickUpper The upper tick of the position for which to burn liquidity
/// @param amount How much liquidity to burn
/// @return amount0 The amount of token0 sent to the recipient
/// @return amount1 The amount of token1 sent to the recipient
function burn(
address recipient,
int24 tickLower,
int24 tickUpper,
uint128 amount
Expand Down
2 changes: 0 additions & 2 deletions contracts/interfaces/pool/IUniswapV3PoolEvents.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,13 @@ interface IUniswapV3PoolEvents {
/// @notice Emitted when a position's liquidity is removed
/// @dev Does not withdraw any fees earned by the liquidity position, which must be withdrawn via #collect
/// @param owner The owner of the position for which liquidity is removed
/// @param recipient The address that receives the tokens withdrawn via the burned liquidity
/// @param tickLower The lower tick of the position
/// @param tickUpper The upper tick of the position
/// @param amount The amount of liquidity to remove
/// @param amount0 The amount of token0 withdrawn
/// @param amount1 The amount of token1 withdrawn
event Burn(
address indexed owner,
address recipient,
int24 indexed tickLower,
int24 indexed tickUpper,
uint128 amount,
Expand Down
8 changes: 4 additions & 4 deletions contracts/interfaces/pool/IUniswapV3PoolState.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,17 @@ interface IUniswapV3PoolState {
/// @return _liquidity The amount of liquidity in the position,
/// Returns feeGrowthInside0LastX128 fee growth of token0 inside the tick range as of the last mint/burn/poke,
/// Returns feeGrowthInside1LastX128 fee growth of token1 inside the tick range as of the last mint/burn/poke,
/// Returns feesOwed0 the computed amount of token0 owed to the position as of the last mint/burn/poke,
/// Returns feesOwed1 the computed amount of token1 owed to the position as of the last mint/burn/poke
/// Returns tokensOwed0 the computed amount of token0 owed to the position as of the last mint/burn/poke,
/// Returns tokensOwed1 the computed amount of token1 owed to the position as of the last mint/burn/poke
function positions(bytes32 key)
external
view
returns (
uint128 _liquidity,
uint256 feeGrowthInside0LastX128,
uint256 feeGrowthInside1LastX128,
uint128 feesOwed0,
uint128 feesOwed1
uint128 tokensOwed0,
uint128 tokensOwed1
);

/// @notice Returns data about a specific observation index
Expand Down
14 changes: 7 additions & 7 deletions contracts/libraries/Position.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ library Position {
uint256 feeGrowthInside0LastX128;
uint256 feeGrowthInside1LastX128;
// the fees owed to the position owner in token0/token1
uint128 feesOwed0;
uint128 feesOwed1;
uint128 tokensOwed0;
uint128 tokensOwed1;
}

/// @notice Returns the Info struct of a position, given an owner and position boundaries
Expand Down Expand Up @@ -58,15 +58,15 @@ library Position {
}

// calculate accumulated fees
uint128 feesOwed0 =
uint128 tokensOwed0 =
uint128(
FullMath.mulDiv(
feeGrowthInside0X128 - _self.feeGrowthInside0LastX128,
_self.liquidity,
FixedPoint128.Q128
)
);
uint128 feesOwed1 =
uint128 tokensOwed1 =
uint128(
FullMath.mulDiv(
feeGrowthInside1X128 - _self.feeGrowthInside1LastX128,
Expand All @@ -79,10 +79,10 @@ library Position {
if (liquidityDelta != 0) self.liquidity = liquidityNext;
self.feeGrowthInside0LastX128 = feeGrowthInside0X128;
self.feeGrowthInside1LastX128 = feeGrowthInside1X128;
if (feesOwed0 > 0 || feesOwed1 > 0) {
if (tokensOwed0 > 0 || tokensOwed1 > 0) {
// overflow is acceptable, have to withdraw before you hit type(uint128).max fees
self.feesOwed0 += feesOwed0;
self.feesOwed1 += feesOwed1;
self.tokensOwed0 += tokensOwed0;
self.tokensOwed1 += tokensOwed1;
}
}
}
2 changes: 1 addition & 1 deletion contracts/test/TestUniswapV3ReentrantCallee.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ contract TestUniswapV3ReentrantCallee is IUniswapV3SwapCallback {
}

// try to reenter burn
try IUniswapV3Pool(msg.sender).burn(address(0), 0, 0, 0) {} catch Error(string memory reason) {
try IUniswapV3Pool(msg.sender).burn(0, 0, 0) {} catch Error(string memory reason) {
require(keccak256(abi.encode(reason)) == keccak256(abi.encode(expectedReason)));
}

Expand Down
18 changes: 9 additions & 9 deletions test/UniswapV3Pool.arbitrage.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,23 +256,23 @@ describe('UniswapV3Pool arbitrage tests', () => {

// burn the arb's liquidity
const { amount0: amount0Burn, amount1: amount1Burn } = await pool.callStatic.burn(
wallet.address,
tickLower,
tickUpper,
getMaxLiquidityPerTick(tickSpacing)
)
await pool.burn(wallet.address, tickLower, tickUpper, getMaxLiquidityPerTick(tickSpacing))
await pool.burn(tickLower, tickUpper, getMaxLiquidityPerTick(tickSpacing))
arbBalance0 = arbBalance0.add(amount0Burn)
arbBalance1 = arbBalance1.add(amount1Burn)

// add the fees as well
const { amount0: amount0Collect, amount1: amount1Collect } = await pool.callStatic.collect(
arbitrageur.address,
tickLower,
tickUpper,
MaxUint128,
MaxUint128
)
const {
amount0: amount0CollectAndBurn,
amount1: amount1CollectAndBurn,
} = await pool.callStatic.collect(arbitrageur.address, tickLower, tickUpper, MaxUint128, MaxUint128)
const [amount0Collect, amount1Collect] = [
amount0CollectAndBurn.sub(amount0Burn),
amount1CollectAndBurn.sub(amount1Burn),
]
arbBalance0 = arbBalance0.add(amount0Collect)
arbBalance1 = arbBalance1.add(amount1Collect)

Expand Down
14 changes: 7 additions & 7 deletions test/UniswapV3Pool.gas.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,18 +241,18 @@ describe('UniswapV3Pool gas tests', () => {
})

it('burn when only position using ticks', async () => {
await snapshotGasCost(pool.burn(wallet.address, tickLower, tickUpper, expandTo18Decimals(1)))
await snapshotGasCost(pool.burn(tickLower, tickUpper, expandTo18Decimals(1)))
})
it('partial position burn', async () => {
await snapshotGasCost(pool.burn(wallet.address, tickLower, tickUpper, expandTo18Decimals(1).div(2)))
await snapshotGasCost(pool.burn(tickLower, tickUpper, expandTo18Decimals(1).div(2)))
})
it('entire position burn but other positions are using the ticks', async () => {
await mint(other.address, tickLower, tickUpper, expandTo18Decimals(1))
await snapshotGasCost(pool.burn(wallet.address, tickLower, tickUpper, expandTo18Decimals(1)))
await snapshotGasCost(pool.burn(tickLower, tickUpper, expandTo18Decimals(1)))
})
it('burn entire position after some time passes', async () => {
await pool.advanceTime(1)
await snapshotGasCost(pool.burn(wallet.address, tickLower, tickUpper, expandTo18Decimals(1)))
await snapshotGasCost(pool.burn(tickLower, tickUpper, expandTo18Decimals(1)))
})
})
}
Expand All @@ -265,9 +265,9 @@ describe('UniswapV3Pool gas tests', () => {
it('best case', async () => {
await mint(wallet.address, tickLower, tickUpper, expandTo18Decimals(1))
await swapExact0For1(expandTo18Decimals(1).div(100), wallet.address)
await pool.burn(wallet.address, tickLower, tickUpper, 0)
await pool.burn(tickLower, tickUpper, 0)
await swapExact0For1(expandTo18Decimals(1).div(100), wallet.address)
await snapshotGasCost(pool.burn(wallet.address, tickLower, tickUpper, 0))
await snapshotGasCost(pool.burn(tickLower, tickUpper, 0))
})
})

Expand All @@ -278,7 +278,7 @@ describe('UniswapV3Pool gas tests', () => {
it('close to worst case', async () => {
await mint(wallet.address, tickLower, tickUpper, expandTo18Decimals(1))
await swapExact0For1(expandTo18Decimals(1).div(100), wallet.address)
await pool.burn(wallet.address, tickLower, tickUpper, 0) // poke to accumulate fees
await pool.burn(tickLower, tickUpper, 0) // poke to accumulate fees
await snapshotGasCost(pool.collect(wallet.address, tickLower, tickUpper, MaxUint128, MaxUint128))
})
})
Expand Down
Loading

0 comments on commit 5515da1

Please sign in to comment.