-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
267043f
to
4fdc4cb
Compare
@@ -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()" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
- if beforeSwapDelta is both negative, it means the hook want to give user extra tokenIn and tokenOut
- if beforeSwapDelta is positive, it means the hook want to take a "fee" in both tokenIn and tokenOut
- 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?
- if int128 is negative, it means the hook want to give amountOut
- if int128 is positive, it means the hook want to take some amountOut as "fee"
There was a problem hiding this comment.
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.
- if beforeSwapDelta is both negative, it means the hook want to take a "fee" in both tokenIn and tokenOut
- if beforeSwapDelta is positive, it means the hook want to give user extra tokenIn and tokenOut
- 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
There was a problem hiding this comment.
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:
ce97d6b
to
b3901e8
Compare
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@@ -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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved -- can you please address https://github.com/pancakeswap/pancake-v4-core/pull/49/files#r1616613856 before merging, seems like the comment for mint()
is not updated
though per discussed -- need to look if we can fix the current discrepancy in beforeSwapDelta between CL and Bin pool in next PR
sorry. Forgot to push the update, can u check again |
@ChefSnoopy @chef-omelette Feel free to add more comments, i will merge for now to unblock incoming optimization |
Can also take a look at the following one (check #50) which alters the logic regarding how we deal with before/afterSwap return value for bin-poo to keep it consistent across pool types |
According to our internal discussion, we've flipped the sign for BalanceDelta to make it less counter-intuitive.