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

Optimize v2 hook #49

Merged
merged 8 commits into from
Aug 10, 2023
Merged

Optimize v2 hook #49

merged 8 commits into from
Aug 10, 2023

Conversation

emmaguo13
Copy link
Contributor

No description provided.

@emmaguo13 emmaguo13 mentioned this pull request Aug 8, 2023
@emmaguo13 emmaguo13 requested a review from ewilz August 9, 2023 19:20
@@ -47,13 +48,7 @@ contract FullRange is BaseHook, ILockCallback {

struct PoolInfo {
uint128 liquidity;
Copy link
Member

Choose a reason for hiding this comment

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

is this liquidity param redundant with the erc20 totalSupply??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah it is! i am thinking of getting rid of this liquidity param anyways.

tickLower: MIN_TICK,
tickUpper: MAX_TICK,
liquidityDelta: -int256(int128(position.liquidity))
})
Copy link
Member

@ewilz ewilz Aug 10, 2023

Choose a reason for hiding this comment

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

confused here....should we be taking out the liquidity as stated by poolManager.getLiquidity() which gets out of sync with our internal position.liquidity here as fees get accumulated? If what I said was true...we woudln't be swapping against a 0 liquidity pool down below....just one with liquidity made up of accrued fees

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, fixing it to use poolManager.getLiquidity(). i could also sync up position.liquidity with the fees, which i accidentally removed :'(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

although it adds 300 gas to rebalancing, i assume syncing position.liquidity will add ~200 gas so maybe not a big deal

Copy link
Member

Choose a reason for hiding this comment

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

yahhh needing to load the context of an external contract I think is at least 200 gas overhead, but I think this makes more sense, and 200 isn't toooo much

@ewilz
Copy link
Member

ewilz commented Aug 10, 2023

codes a lot cleaner!

@emmaguo13 emmaguo13 marked this pull request as ready for review August 10, 2023 15:08
@emmaguo13 emmaguo13 merged commit 014578d into v2-hook Aug 10, 2023
2 checks passed
@emmaguo13 emmaguo13 deleted the opt-v2-hook branch August 10, 2023 15:08
emmaguo13 added a commit that referenced this pull request Aug 17, 2023
* in progress full range

* rename

* in progress

* compiling

* basic tests passing

* erc20 contract deploy

* initial liquidity deposit working

* removeLiquidity tests

* withdrawing liquidity works

* merge with main

* in progress

* compiling code with rebalancing

* basic liquidity add tests with fee working

* in progress add liq swap tests

* debug add liquidity rebalance

* simple rebalance working

* removing liquidity tests passing but check implementation

* add gas snapshots

* add blockNumber and liquidity token addr to poolinfo struct, update gas snapshots

* update erc20 contract, rerun gas snapshots, remove create2

* readd create2, rerun gas snapshots with create2

* increase amounts used in tests

* update tests and debug rebalance

* fixed rebalancing dust issue

* working tests and fixed rebalancing

* fix dependencies

* debug lib

* some code cleanup and v4-core update

* some more cleanup

* some more code cleanup

* Optimize v2 hook (#49)

* sload and swrite on every swap

* sload on every swap, only write if need to

* remove console.logs

* rebalance and reinvest dust with donate

* remove no rebalance test

* some code cleanup

* test cleanup and store poolId

* remove liquidity in pool info struct

* comment + test cleanup

* deploy erc20 with custom liquidity token name

* cleanup and add custom tick spacing error

* add minimum liquidity

* make rebalance public

* fuzz testing

* clean up tests

* in progress pr comment addressing

* fix burning liquidity, some code cleanup

* uncached storage slot remove liquidity gas test

* use safe cast library

* fix minimum liquidity error, add price slippage test

* address pr comments

* fix remove liquidity fuzz test

* add fuzz test for swapping and removing all liquidity

* add univ4 to erc20 token names

* update init snapshot

* fix max tick liquidity

* custom errors
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