update decrease #133
Merged
update decrease #133
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
commented
Jun 28, 2024
| 177014 |
snreynolds
commented
Jun 28, 2024
| function addLiquidity(IBaseLiquidityManagement.Position storage position, uint256 liquidity) internal { | ||
| unchecked { | ||
| position.liquidity += liquidity; | ||
| } | ||
| } | ||
|
|
||
| function subtractLiquidity(IBaseLiquidityManagement.Position storage position, uint256 liquidity) internal { | ||
| if (position.liquidity < liquidity) revert InsufficientLiquidity(); |
There was a problem hiding this comment.
We could probably move this outside of here tbh and check it sooner but for now fine?
There was a problem hiding this comment.
i dont mind it here, gives a nice assurance with the following unchecked block
saucepoint
approved these changes
Jun 28, 2024
Comment on lines
+290
to
+291
| BalanceDelta tokensOwed = | ||
| toBalanceDelta(uint256(position.tokensOwed0).toInt128(), uint256(position.tokensOwed1).toInt128()); |
There was a problem hiding this comment.
another reason to move Positon.tokensOwed0/Position.tokensOwed1 to BalanceDelta? 🤨
Comment on lines
+233
to
+236
| if (callerDelta.amount1() > 0) { | ||
| (tokensOwed, callerDelta, thisDelta) = | ||
| _moveCallerDeltaToTokensOwed(false, tokensOwed, callerDelta, thisDelta); | ||
| } |
There was a problem hiding this comment.
im certain that the callerDelta.amount1() > 0 isnt needed since decrease liquidity should always create a credit / positive delta
noting for myself and can fix with transient library
There was a problem hiding this comment.
Nice yeah, I think I put that there in case it ever was exactly 0 to just skip all that..
since technically someone could call decrease with liquidityAmount = 0?
| import "forge-std/console2.sol"; | ||
|
|
||
| library LiquidityDeltaAccounting { | ||
| function split(BalanceDelta liquidityDelta, BalanceDelta callerFeesAccrued, BalanceDelta totalFeesAccrued) |
There was a problem hiding this comment.
i like this library a lot and how its used. its a super minor nit but i think we should call it discern and switch the parameters so the syntax becomes:
liquidityDelta.discern(totalFeesAccrued, callerFeesAccrued)
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