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

refactor: flip the sign for balanceDelta #49

Merged
merged 4 commits into from
May 29, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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/BinHookTest#testBurnSucceedsWithHook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
182049
181729
Original file line number Diff line number Diff line change
@@ -1 +1 @@
185296
185616
2 changes: 1 addition & 1 deletion .forge-snapshots/BinHookTest#testMintSucceedsWithHook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
331250
331570
Original file line number Diff line number Diff line change
@@ -1 +1 @@
138533
138277
Original file line number Diff line number Diff line change
@@ -1 +1 @@
32563
32575
Original file line number Diff line number Diff line change
@@ -1 +1 @@
148635
148315
Original file line number Diff line number Diff line change
@@ -1 +1 @@
296486
296230
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
131743
131487
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasDonate.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
120042
120359
Original file line number Diff line number Diff line change
@@ -1 +1 @@
977200
977517
Original file line number Diff line number Diff line change
@@ -1 +1 @@
339390
339707
Original file line number Diff line number Diff line change
@@ -1 +1 @@
342867
343184
Original file line number Diff line number Diff line change
@@ -1 +1 @@
145810
146127
Original file line number Diff line number Diff line change
@@ -1 +1 @@
309383
309700
Original file line number Diff line number Diff line change
@@ -1 +1 @@
362972
362978
Original file line number Diff line number Diff line change
@@ -1 +1 @@
178140
178146
Original file line number Diff line number Diff line change
@@ -1 +1 @@
248255
248182
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#donateBothTokens.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
170425
170745
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#gasDonateOneToken.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
114348
114589
Original file line number Diff line number Diff line change
@@ -1 +1 @@
124482
124243
Original file line number Diff line number Diff line change
@@ -1 +1 @@
141710
141453
Original file line number Diff line number Diff line change
@@ -1 +1 @@
175389
174968
Original file line number Diff line number Diff line change
@@ -1 +1 @@
25094185
161331
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_simple.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
79576
79416
Original file line number Diff line number Diff line change
@@ -1 +1 @@
153925
153614
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_withHooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
96383
96223
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_withNative.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
79579
79419
Original file line number Diff line number Diff line change
@@ -1 +1 @@
32291
32279
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2237
2239
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3022
3029
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1994
1981
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3022
3029
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2227
2229
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3175
3182
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1984
1971
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3175
3182
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#Vault.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
7136
7171
Original file line number Diff line number Diff line change
@@ -1 +1 @@
81625
81624
28 changes: 12 additions & 16 deletions src/Vault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,13 @@ contract Vault is IVault, VaultToken, Ownable {

/// @inheritdoc IVault
function take(Currency currency, address to, uint256 amount) external override isLocked {
SettlementGuard.accountDelta(msg.sender, currency, amount.toInt128());
SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128()));
currency.transfer(to, amount);
}

/// @inheritdoc IVault
function mint(address to, Currency currency, uint256 amount) external override isLocked {
SettlementGuard.accountDelta(msg.sender, currency, amount.toInt128());
SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128()));
_mint(to, currency, amount);
}

Expand All @@ -128,22 +128,21 @@ contract Vault is IVault, VaultToken, Ownable {
paid = msg.value;
}

// subtraction must be safe
SettlementGuard.accountDelta(msg.sender, currency, -(paid.toInt128()));
SettlementGuard.accountDelta(msg.sender, currency, paid.toInt128());
}

/// @inheritdoc IVault
function settleFor(Currency currency, address target, uint256 amount) external isLocked {
/// @notice settle all outstanding debt if amount is 0
/// It will revert if target has negative delta
if (amount == 0) amount = SettlementGuard.getCurrencyDelta(target, currency).toUint256();
SettlementGuard.accountDelta(msg.sender, currency, amount.toInt128());
SettlementGuard.accountDelta(target, currency, -(amount.toInt128()));
/// It will revert if target has positive delta
Copy link
Collaborator

@ChefMist ChefMist May 28, 2024

Choose a reason for hiding this comment

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

curious, what does this comment mean. does it mean that it will revert eventually in the transaction if the target has non-zero delta?

eg. the comment might imply that if target has positive delta, this method will revert

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when calling settleFor, msg.sender is trying to cover the payment for target right ?

If the target has a positive delta, it means that the target doesn't owe anything to the pool and, in fact, the pool owes tokens to them..(after updating everything flip) In this case, pass in amount=0 (i.e. ettle all outstanding debt for target) is invalid.

Copy link
Collaborator

@ChefMist ChefMist May 28, 2024

Choose a reason for hiding this comment

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

when calling settleFor, msg.sender is trying to cover the payment for target right ?

I guess when there's a comment like /// It will revert if target has positive delta -- people might think there's a revert in this settleFor method. But its basically the CurrencyNotSettled() revert that happens at the end right? or is there actually a validation for this target has positive delta specifically?

If the comment is correct, would it mean we can write the same comment in take/settle etc.. it will revert if caller has positive delta as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

“But its basically the CurrencyNotSettled() revert that happens at the end right? or is there actually a validation for this target has positive delta specifically?”

No, it will revert as in SafeCast.toUint256 because we cant cast a negative number (given delta is positive, minus it results in negative) to uint256

CleanShot 2024-05-28 at 15 58 06@2x

if (amount == 0) amount = (-SettlementGuard.getCurrencyDelta(target, currency)).toUint256();
SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128()));
SettlementGuard.accountDelta(target, currency, amount.toInt128());
}

/// @inheritdoc IVault
function burn(address from, Currency currency, uint256 amount) external override isLocked {
SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128()));
SettlementGuard.accountDelta(msg.sender, currency, amount.toInt128());
_burnFrom(from, currency, amount);
}

Expand All @@ -162,14 +161,11 @@ contract Vault is IVault, VaultToken, Ownable {
if (delta == 0) return;

if (delta >= 0) {
reservesOfPoolManager[poolManager][currency] += uint128(delta);
/// @dev arithmetic underflow make sure trader can't withdraw too much from poolManager
reservesOfPoolManager[poolManager][currency] -= uint128(delta);
} else {
/// @dev arithmetic underflow is possible in following two cases:
/// 1. delta == type(int128).min
/// This occurs when withdrawing amount is too large
/// 2. reservesOfPoolManager[poolManager][currency] < delta0
/// This occurs when insufficient balance in pool
reservesOfPoolManager[poolManager][currency] -= uint128(-delta);
/// @dev arithmetic overflow make sure trader won't deposit too much into poolManager
reservesOfPoolManager[poolManager][currency] += uint128(-delta);
}
}
}
6 changes: 3 additions & 3 deletions src/pool-bin/interfaces/IBinHooks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ interface IBinHooks is IHooks {
/// @param sender The initial msg.sender for the modify position call
/// @param key The key for the pool
/// @param params The parameters for adding liquidity
/// @param delta The amount owed to the locker (negative) or owed to the pool (positive)
/// @param delta The amount owed to the locker (positive) or owed to the pool (negative)
/// @param hookData Arbitrary data handed into the PoolManager by the liquidty provider to be be passed on to the hook
/// @return bytes4 The function selector for the hook
/// @return BalanceDelta The hook's delta in token0 and token1.
Expand Down Expand Up @@ -93,7 +93,7 @@ interface IBinHooks is IHooks {
/// @param sender The initial msg.sender for the modify position call
/// @param key The key for the pool
/// @param params The parameters for removing liquidity
/// @param delta The amount owed to the locker (negative) or owed to the pool (positive)
/// @param delta The amount owed to the locker (positive) or owed to the pool (negative)
/// @param hookData Arbitrary data handed into the PoolManager by the liquidty provider to be be passed on to the hook
/// @return bytes4 The function selector for the hook
/// @return BalanceDelta The hook's delta in token0 and token1.
Expand Down Expand Up @@ -126,7 +126,7 @@ interface IBinHooks is IHooks {
/// @param key The key for the pool
/// @param swapForY If true, indicate swap X for Y or if false, swap Y for X
/// @param amountIn Amount of tokenX or tokenY in
/// @param delta The amount owed to the locker (negative) or owed to the pool (positive)
/// @param delta The amount owed to the locker (positive) or owed to the pool (negtive)
/// @param hookData Arbitrary data handed into the PoolManager by the swapper to be be passed on to the hook
/// @return bytes4 The function selector for the hook
/// @return int128 The hook's delta in unspecified currency
Expand Down
2 changes: 1 addition & 1 deletion src/pool-bin/interfaces/IBinPoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ interface IBinPoolManager is IProtocolFees, IPoolManager, IExtsload {
returns (BalanceDelta delta);

/// @notice Donate the given currency amounts to the pool with the given pool key.
/// @return delta Positive amt means the caller owes the vault, while negative amt means the vault owes the caller
/// @return delta Negative amt means the caller owes the vault, while positive amt means the vault owes the caller
/// @return binId The donated bin id, which is the current active bin id. if no-op happen, binId will be 0
function donate(PoolKey memory key, uint128 amount0, uint128 amount1, bytes calldata hookData)
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's also the mint() comment around delta that neeed to flipped

 /// @return delta BalanceDelta, will be positive indicating how much total amt0 and amt1 liquidity added

to

 /// @return delta BalanceDelta, will be negative indicating how much total amt0 and amt1 liquidity added

external
Expand Down
12 changes: 7 additions & 5 deletions src/pool-bin/libraries/BinHooks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,10 @@ library BinHooks {

if (hookDeltaSpecified != 0) {
/// @dev default overflow check make sure the swap amount is always valid
if (hookDeltaSpecified > 0) {
amountToSwap += uint128(hookDeltaSpecified);
} else {
if (hookDeltaSpecified < 0) {
amountToSwap -= uint128(-hookDeltaSpecified);
} else {
amountToSwap += uint128(hookDeltaSpecified);
}
}
}
Expand Down Expand Up @@ -163,9 +163,11 @@ library BinHooks {
hookDeltaUnspecified += beforeSwapDelta.getUnspecifiedDelta();

if (hookDeltaUnspecified != 0 || hookDeltaSpecified != 0) {
// the hookDelta is the delta added into amountIn which is in the opposite direction of the balanceDelta
// i.e. in a reasonable case, amountIn will basiaclly equal to "-BalanceDelta.amount0()/amount1()"
Copy link
Collaborator Author

@chefburger chefburger May 28, 2024

Choose a reason for hiding this comment

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

This is now completely opposite when comparing with cl-pool because for cl-pool when amountSpecified > 0 it means we are specifying amountOut, while for bin-pool we can only specify amountIn and it's always positive.

That's why in the following call i.e. toBalanceDelta we flip the sign for both parts

Copy link
Collaborator

@ChefMist ChefMist May 28, 2024

Choose a reason for hiding this comment

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

want to clarify: does the hook for both Bin and CL have the same behavour for their return value? eg. BeforeSwapDelta in beforeSwap and int128 at afterSwap()?

eg.

1/ what does negative BeforeSwapDelta for both value means?
2/ what does negative int128 in afterSwap means?

Copy link
Collaborator

Choose a reason for hiding this comment

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

is this right?

1/ what does negative BeforeSwapDelta for both value means?

  1. if beforeSwapDelta is both negative, it means the hook want to give user extra tokenIn and tokenOut
  2. if beforeSwapDelta is positive, it means the hook want to take a "fee" in both tokenIn and tokenOut
  3. likewise if its (positive , negative) it means hook want to take tokenIn as fee and give extra tokenOut

2/ what does negative int128 in afterSwap means?

  1. if int128 is negative, it means the hook want to give amountOut
  2. if int128 is positive, it means the hook want to take some amountOut as "fee"

Copy link
Collaborator Author

@chefburger chefburger May 28, 2024

Choose a reason for hiding this comment

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

for bin-pool, I think it should be opposite.

  1. if beforeSwapDelta is both negative, it means the hook want to take a "fee" in both tokenIn and tokenOut
  2. if beforeSwapDelta is positive, it means the hook want to give user extra tokenIn and tokenOut
  3. likewise if its (positive , negative) it means hook want to give extra tokenIn and take tokenOut as fee

Let's elaborate case 3 for example:

if BeforeSwapDelta.getSpecifiedDelta() > 0, i.e. amountToSwap is even bigger than trader's specification, so more amountIn needed for this swap, and let's see who will pay for the extra part:

from trader perspective:

The amountIn delta returned from pool.swap is definitely bigger in abs value because amountIn is bigger, but remember the delta is updated through delta - hookDelta in afterSwap

and we know hookDelta = (-hookDeltaSpecified, -hookDeltaUnspecified)
a negative value minus another negative value will result in a bigger negative value i.e. user are actually not paying that part

from hook perspective:

if (hookDelta != BalanceDeltaLibrary.ZERO_DELTA) {
    vault.accountPoolBalanceDelta(key, hookDelta, address(key.hooks));
}

This will count a negative amountIn delta on hook, so inside hook callback there must be a payment to the pool

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

2/ what does negative int128 in afterSwap means?

Also, should be opposite
if int128 is positive, it means the hook want to give amountOut
if int128 is negative, it means the hook want to take some amountOut as "fee"

In fact
int128 in afterSwap and BeforeSwapDelta.getUnspecifiedDelta() in beforeSwap are actually specifying the same thing..

can see they are combined in afterSwap before generating the final hookDelta:

CleanShot 2024-05-28 at 15 14 01@2x

hookDelta = swapForY
? toBalanceDelta(hookDeltaSpecified, hookDeltaUnspecified)
: toBalanceDelta(hookDeltaUnspecified, hookDeltaSpecified);
? toBalanceDelta(-hookDeltaSpecified, -hookDeltaUnspecified)
: toBalanceDelta(-hookDeltaUnspecified, -hookDeltaSpecified);

// the caller has to pay for (or receive) the hook's delta
swapperDelta = delta - hookDelta;
Expand Down
13 changes: 7 additions & 6 deletions src/pool-bin/libraries/BinPool.sol
Original file line number Diff line number Diff line change
Expand Up @@ -261,10 +261,10 @@ library BinPool {

if (swapForY) {
uint128 consumed = amountIn - amountsLeft.decodeX();
result = toBalanceDelta(consumed.safeInt128(), -(amountsOut.decodeY().safeInt128()));
result = toBalanceDelta(-consumed.safeInt128(), amountsOut.decodeY().safeInt128());
} else {
uint128 consumed = amountIn - amountsLeft.decodeY();
result = toBalanceDelta(-(amountsOut.decodeX().safeInt128()), consumed.safeInt128());
result = toBalanceDelta(amountsOut.decodeX().safeInt128(), -(consumed.safeInt128()));
}
}

Expand Down Expand Up @@ -303,7 +303,9 @@ library BinPool {
compositionFee = compoFee;

(uint128 x1, uint128 x2) = params.amountIn.sub(amountsLeft).decode();
result = toBalanceDelta(x1.safeInt128(), x2.safeInt128());

// set balanceDelta to negative (so user must settle()) from the vault
result = toBalanceDelta(-(x1.safeInt128()), -(x2.safeInt128()));
}

/// @notice Returns the reserves of a bin
Expand Down Expand Up @@ -372,8 +374,7 @@ library BinPool {
}
}

// set amoutsOut to negative (so user can take/mint()) from the vault
result = toBalanceDelta(-(amountsOut.decodeX().safeInt128()), -(amountsOut.decodeY().safeInt128()));
result = toBalanceDelta(amountsOut.decodeX().safeInt128(), amountsOut.decodeY().safeInt128());
}

function donate(State storage self, uint16 binStep, uint128 amount0, uint128 amount1)
Expand All @@ -391,7 +392,7 @@ library BinPool {
binReserves.add(amountIn).getLiquidity(price);

self.reserveOfBin[activeId] = binReserves.add(amountIn);
result = toBalanceDelta(amount0.safeInt128(), amount1.safeInt128());
result = toBalanceDelta(-(amount0.safeInt128()), -(amount1.safeInt128()));
}

/// @dev Helper function to mint liquidity in each bin in the liquidity configurations
Expand Down
2 changes: 2 additions & 0 deletions src/pool-cl/interfaces/ICLHooks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ interface ICLHooks is IHooks {
/// @param sender The initial msg.sender for the add liquidity call
/// @param key The key for the pool
/// @param params The parameters for adding liquidity
/// @param delta The amount owed to the locker (positive) or owed to the pool (negative)
/// @param hookData Arbitrary data handed into the PoolManager by the liquidty provider to be be passed on to the hook
/// @return bytes4 The function selector for the hook
/// @return BalanceDelta The hook's delta in token0 and token1.
Expand Down Expand Up @@ -99,6 +100,7 @@ interface ICLHooks is IHooks {
/// @param sender The initial msg.sender for the remove liquidity call
/// @param key The key for the pool
/// @param params The parameters for removing liquidity
/// @param delta The amount owed to the locker (positive) or owed to the pool (negative)
/// @param hookData Arbitrary data handed into the PoolManager by the liquidty provider to be be passed on to the hook
/// @return bytes4 The function selector for the hook
/// @return BalanceDelta The hook's delta in token0 and token1.
Expand Down
6 changes: 3 additions & 3 deletions src/pool-cl/libraries/CLHooks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,9 @@ library CLHooks {
int128 hookDeltaSpecified = beforeSwapDelta.getSpecifiedDelta();

if (hookDeltaSpecified != 0) {
bool exactInput = amountToSwap > 0;
bool exactInput = amountToSwap < 0;
amountToSwap += hookDeltaSpecified;
if (exactInput ? amountToSwap < 0 : amountToSwap > 0) revert Hooks.HookDeltaExceedsSwapAmount();
if (exactInput ? amountToSwap > 0 : amountToSwap < 0) revert Hooks.HookDeltaExceedsSwapAmount();
}
}
}
Expand Down Expand Up @@ -147,7 +147,7 @@ library CLHooks {
hookDeltaUnspecified += beforeSwapDelta.getUnspecifiedDelta();

if (hookDeltaUnspecified != 0 || hookDeltaSpecified != 0) {
hookDelta = (params.amountSpecified > 0 == params.zeroForOne)
hookDelta = (params.amountSpecified < 0 == params.zeroForOne)
? toBalanceDelta(hookDeltaSpecified, hookDeltaUnspecified)
: toBalanceDelta(hookDeltaUnspecified, hookDeltaSpecified);

Expand Down
Loading
Loading