change add liq accounting #126
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
snreynolds
force-pushed
the
sra/addLiq-accounting-updates
branch
from
220efb8 to
9edb230
Compare
saucepoint
reviewed
Jun 20, 2024
contracts/libraries/Position.sol
Outdated
Comment on lines
9
to
16
| function updateTokensOwed( | ||
| IBaseLiquidityManagement.Position storage position, | ||
| uint128 tokensOwed0, | ||
| uint128 tokensOwed1 | ||
| ) internal { | ||
| position.tokensOwed0 = tokensOwed0; | ||
| position.tokensOwed1 = tokensOwed1; | ||
| } |
There was a problem hiding this comment.
Ive been thinking about moving tokensOwed0/tokensOwed1 to a BalanceDelta type. iirc we almost always handle the two as a pair
maybe we're over-using the type, but i like it a lot haha
Comment on lines
+337
to
+338
| (token0Owed) = FeeMath.getFeeOwed(feeGrowthInside0X128, position.feeGrowthInside0LastX128, position.liquidity); | ||
| (token1Owed) = FeeMath.getFeeOwed(feeGrowthInside1X128, position.feeGrowthInside1LastX128, position.liquidity); |
There was a problem hiding this comment.
any reason why to call this twice vs the getFeesOwed which returns both?
| BalanceDelta totalFeesAccrued, | ||
| BalanceDelta callerFeesAccrued, | ||
| PoolKey memory poolKey | ||
| ) internal returns (int128 callerDelta0, int128 callerDelta1) { |
There was a problem hiding this comment.
another candidate to move the return type to BalanceDelta ?
saucepoint
reviewed
Jun 24, 2024
There was a problem hiding this comment.
directionally i think its getting there. makes a lot of sense to separately handle callerDeltas vs externalDeltas and remove PM currency transfers within the operation functions (_increaseLiq, _decreaseLiq, _collect). feels like we can reduce the round trips to PM
have picked up on some edge cases we might need to think thru:
Comment on lines
200
to
201
| int128 delta0 = int128(manager.currencyDelta(address(this), currency0)); | ||
| int128 delta1 = int128(manager.currencyDelta(address(this), currency1)); |
There was a problem hiding this comment.
you can use CurrencyDeltas, which uses sparse extload to load both deltas in a single external call 😎
There was a problem hiding this comment.
can we move that function to TransientStateLibrary.sol ?
* additional increase liquidity tests * edge case of using cached fees for autocompound * wip
* fix some assertions * use BalanceDeltas for arithmetic * cleanest code in the game??? * additional cleaning * typo lol * autocompound gas benchmarks * autocompound excess credit gas benchmark * save 600 gas, cleaner code when moving caller delta to tokensOwed
saucepoint
approved these changes
Jun 26, 2024
snreynolds
added a commit
that referenced
this pull request
Jul 23, 2024
* initial thoughts lock and batch * update safecallback with constructor * simple batch under lock * oops * misc version bump; will conflict but can resolve later * defining types and different levels of abstractions * merge in main; resolve conflicts * wip * misc fixes with main:latest * basic mint * begin moving tests to fuzz * test for slippage * burning * decrease liquidity * mint transfer burn, liquidityOf accounting * wip * refactor to use CurrencySettleTake * basic fee collection * wip * misc fix * fee collection for independent same-range parties * LiquidityPosition -> LiquidityRange * erc20 fee collection * decrease liquidity with fee collection * wip test decrease liquidity on same range * reworked fuzzers; more testing on fee claims for liquidity decreasing * forge fmt * test fixes for flipped deltas * wip * test coverage for increase liquidity cases * preliminary gas benchmarks * Position manager refactor (#2) * chore: update v4-core:latest (#105) * update core * rename lockAcquired to unlockCallback * update core; temporary path hack in remappings * update v4-core; remove remapping * wip: fix compatibility * update core; fix renaming of swap fee to lp fee * update core; fix events * update core; address liquidity salt and modify liquidity return values * fix incorrect delta accounting when modifying liquidity * fix todo, use CurrencySettleTake * remove deadcode * update core; use StateLibrary; update sqrtRatio to sqrtPrice * fix beforeSwap return signatures * forge fmt; remove commented out code * update core (wow gas savings) * update core * update core * update core; hook flags LSB * update core * update core * chore: update v4 core (#115) * Update v4-core * CurrencySettleTake -> CurrencySettler * Snapshots * compiling but very broken * replace PoolStateLibrary * update currency settle take * compiling * wip * use v4-core's forge-std * test liquidity increase * additional fixes for collection and liquidity decrease * test migration * replace old implementation with new --------- Signed-off-by: saucepoint <[email protected]> Co-authored-by: 0x57 <[email protected]> * cleanup: TODOs and imports * Position manager Consolidate (#3) * wip: consolidation * further consolidation * consolidate to single file * yay no more stack too deep * some code comments * use currency settler syntax * use v4-core's gas snapshot * use snapLastCall and isolate for posm benchmarks * Update contracts/libraries/CurrencySettleTake.sol Co-authored-by: 0x57 <[email protected]> * use v4-core's solmate its more recent * use v4-core's openzeppelin-contracts * add ERC721Permit * feedback: memory hookData * initial refactor. stack too deep * passing tests * gutted LockAndBatchCall * cleanup diff * renaming vanilla functions * sanitize * change add liq accounting (#126) * change add liq accounting * remove rand comments * fix exact fees * use closeAllDeltas * comments cleanup * additional liquidity tests (#129) * additional increase liquidity tests * edge case of using cached fees for autocompound * wip * fix autocompound bug, use custodied and unclaimed fees in the autocompound * fix tests and use BalanceDeltas (#130) * fix some assertions * use BalanceDeltas for arithmetic * cleanest code in the game??? * additional cleaning * typo lol * autocompound gas benchmarks * autocompound excess credit gas benchmark * save 600 gas, cleaner code when moving caller delta to tokensOwed --------- Co-authored-by: saucepoint <[email protected]> * create compatibility with arbitrary execute handler * being supporting batched ops on vanilla functions * some initial tests to drive TDD * gas with isolate * mint to recipient * refactor for external call and code reuse (#134) * updated interface with unlockAndExecute * update decrease (#133) * update decrease * update collect * update decrease/collect * remove delta function * update burn * fix bubbling different return types because of recursive calls * all operations only return a BalanceDelta type (#136) * temp-dev-update (#135) * checkpointing * move decrease and collect to transient storage * remove returns since they are now saved to transient storage * draft: delta closing * wip * Sra/edits (#137) * consolidate using owner, update burn * fix: accrue deltas to caller in increase * Rip Out Vanilla (#138) * rip out vanilla and benchmark * fix gas benchmark * check posm is the locker before allowing access to external functions * restore execute tests * posm takes as 6909; remove legacy deadcode * restore tests * move helpers to the same file * fix: cleanup --------- Co-authored-by: Sara Reynolds <[email protected]> Co-authored-by: Sara Reynolds <[email protected]> * using internal calls, first pass (#143) * using internal calls, first pass * fix gas tests * fix execute test * fix tests * edit mint gas test * fix mint test * fix warnings * dumb fix to test ci (#146) * dumb fix to test ci * memory limit * update gas limit to pass tests --------- Co-authored-by: gretzke <[email protected]> * some more gas snapshots (#150) * feat: posm, use salts for all positions & update permit (#148) * use position salts * use fees owed in some tests * remove claims from increase,decrease * increment token id before reading it * Revert "increment token id before reading it" This reverts commit d366d75. * owner to alice * add more mint gas tests * update comment * Owner-level ERC721 Permit (#153) * checkpointing * move decrease and collect to transient storage * remove returns since they are now saved to transient storage * draft: delta closing * wip * Sra/edits (#137) * consolidate using owner, update burn * fix: accrue deltas to caller in increase * Rip Out Vanilla (#138) * rip out vanilla and benchmark * fix gas benchmark * check posm is the locker before allowing access to external functions * restore execute tests * posm takes as 6909; remove legacy deadcode * restore tests * move helpers to the same file * move operator to NFTposm; move nonce to ERC721Permit; owner-level nonce * tests for operator/permit * snapshots * gas benchmarks for permit * test fixes * unordered nonces * fix tests / cheatcode usage * fix tests --------- Co-authored-by: Sara Reynolds <[email protected]> --------- Co-authored-by: gretzke <[email protected]> Co-authored-by: saucepoint <[email protected]> * Multicall & initialize (#154) * add multicall and an external function for initialization, with tests * test multicall contract * gas snapshot multicall * fix ci test * fix tests * forge fmt * change how msg.value is used in multicall mock --------- Co-authored-by: Sara Reynolds <[email protected]> * prep shared actions (#158) * update main (#162) * ERC721Permit cleanup (#161) * wip Solmate ERC721 * the queens dead, you may put down your arms. with this commit, i hereby lay BaseLiquidityManagement and the ideals of fee accounting finally to rest * refactor tokenId => LiquidityRange; begin separating PoolKey * move comment --------- Co-authored-by: Sara Reynolds <[email protected]> * remove old files, imports * Update src/NonfungiblePositionManager.sol Co-authored-by: saucepoint <[email protected]> * pr comments * pr comments * refactor test helpers per feedback * fix gas * remove permit * fix compiler warnings * rename to PositionManager * cache length * skip take for 0 * fix tests * Update src/interfaces/IPositionManager.sol Co-authored-by: Alice <[email protected]> * update multicall tests per feedback * remove unused imports * more unused imports * improve assertion * assert mint recipient is the payer and not the recipient * pr feedback * assert pool creation * use poolManager * remove liquidityrange imports * remove version string * pr comments, use base test setup * fuzz sqrtPrice * use fuzz, assert eq * other final rand pr comment fixes * fix off by 1 * use bound * use nextTokenId --------- Signed-off-by: saucepoint <[email protected]> Co-authored-by: saucepoint <[email protected]> Co-authored-by: saucepoint <[email protected]> Co-authored-by: 0x57 <[email protected]> Co-authored-by: 0x57 <[email protected]> Co-authored-by: gretzke <[email protected]> Co-authored-by: Alice <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Issue
Which issue does this pull request resolve?
Description of changes