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

chore: Update with latest v4-core changes #32

Merged
merged 6 commits into from
Jun 6, 2024
Merged

Conversation

ChefMist
Copy link
Collaborator

@ChefMist ChefMist commented Jun 5, 2024

Context

In the latest v4-core, a few changes has been made that impacts v4-periphery

  1. flip BalanceDelta and flip amountIn eg. exactInput is when amountSpecified is negative: refactor: flip the sign for balanceDelta pancake-v4-core#49

  2. migrate over to transient storage for vault reserve Refactor: reserve of transient pancake-v4-core#42

  3. adding salt() concept to position: [PART-1] enable hooks to return deltas to influence pool behavior pancake-v4-core#44

  4. swap returns 2 balanceDelta now: [PART-1] enable hooks to return deltas to influence pool behavior pancake-v4-core#44

  5. modifyLiquidity to also return 2 balanceDelta

Changes

  1. the objective is to ensure the code is updated and test passing -- no optimisation has been made to ensure this PR stays as small as possible

Example of optimization required down the road

  1. no longer need to call accumulateFee to claim fee first before adding/removing liqudiity
  2. usage of salt() potentially: Allow both CLMigrator and BinMigrator to be paused  pancake-v4-periphery#24 - right now its just bytes32(0)

@ChefMist ChefMist mentioned this pull request Jun 6, 2024
@ChefMist ChefMist changed the title <WIP> chore: Update with latest v4-core changes chore: Update with latest v4-core changes Jun 6, 2024
@ChefMist ChefMist requested review from ChefSnoopy, chefburger, ChefCupcake and chef-omelette and removed request for ChefSnoopy June 6, 2024 03:18
@@ -165,13 +166,13 @@ abstract contract CLSwapRouterBase is SwapRouterBase, ICLSwapRouterBase {
);

if (zeroForOne) {
reciprocalAmount = amountSpecified > 0 ? delta.amount1() : delta.amount0();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

might need some 👀 on this reciprocalAmount

reciprocalAmount = amountSpecified > 0 ? delta.amount1() : delta.amount0();
if (settle) _payAndSettle(poolKey.currency0, payer, delta.amount0());
if (take) vault.take(poolKey.currency1, recipient, uint128(-delta.amount1()));
reciprocalAmount = amountSpecified < 0 ? -delta.amount1() : -delta.amount0();
Copy link
Collaborator

@chefburger chefburger Jun 6, 2024

Choose a reason for hiding this comment

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

is it better to not have minus here and flip sign for line 28, 62, 92, 123 ?

Reason: the return value reciprocalAmount is the delta of unspecified token according to the context, hence its sign should be aligned with BalanceDelta 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.

i created a PR to further update -- pancakeswap/pancake-v4-periphery#34 can you see if this PR make sense? will merge to this branch if it make sense

@ChefMist
Copy link
Collaborator Author

ChefMist commented Jun 6, 2024

pancakeswap/pancake-v4-periphery@60aeb60 changes is fine

chefburger
chefburger previously approved these changes Jun 6, 2024
@ChefMist
Copy link
Collaborator Author

ChefMist commented Jun 6, 2024

@chef-omelette @ChefSnoopy @ChefCupcake feel free to continue leaving comment, will address them. context: theres still more PR to come to further optimise our codebase due to the changes in v4-core

@ChefMist ChefMist merged commit 33f3fd2 into main Jun 6, 2024
2 checks passed
@ChefMist ChefMist deleted the chore/update-v4-core branch June 6, 2024 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants