Refactor deposit tracking#53
Conversation
garyghayrat
force-pushed
the
feat/change-deposit-tracking
branch
from
87ab860 to
0e729b0
Compare
| /// @param _amount0Max The maximum amount of token0 to deposit across all positions. | ||
| /// @param _amount1Max The maximum amount of token1 to deposit across all positions. | ||
| /// @param _liquidity The liquidity to add to position0. Other positions calculated | ||
| /// proportionally. @dev Requires liquidity to be initialized first via initializeLiquidity(). |
There was a problem hiding this comment.
| /// proportionally. @dev Requires liquidity to be initialized first via initializeLiquidity(). | |
| /// proportionally. | |
| /// @dev Requires liquidity to be initialized first via initializeLiquidity(). |
| /// @return _amount0Used The actual amount of token0 used. | ||
| /// @return _amount1Used The actual amount of token1 used. | ||
| function deposit(uint256 _amount0Max, uint256 _amount1Max) | ||
| function initializeLiquidity(uint256 _amount0Max, uint256 _amount1Max) |
There was a problem hiding this comment.
Since this is called once, and we don't want the caller to put arbitrary values, maybe these are defined in the constructor and are immutable?
|
|
||
| uint256 _totalAmount0Ref; | ||
| uint256 _totalAmount1Ref; | ||
| uint256 _liquidityReference = 1e18; |
There was a problem hiding this comment.
What is this liquidity reference?
There was a problem hiding this comment.
it's just an arbitrary scale reference to compute reference token amounts per position.
There was a problem hiding this comment.
I think the problem here is that this is the liquidity specified for the amounts, so what if this greater than the max? Is the idea the user has to increase their amount max call?
|
|
||
| if (_totalAmount0Ref == 0 && _totalAmount1Ref == 0) revert LPVault__ZeroLiquidity(); | ||
|
|
||
| uint256 _liquidity0Full = type(uint256).max; |
There was a problem hiding this comment.
Why is the default value here max uint?
There was a problem hiding this comment.
Because if we start with zero, down the line, if (_liquidity0ByAmount1 < _liquidity0Full) would return false when _totalAmount1Ref > 0. But usually we want to assign _liquidity0ByAmount1 to it.
There was a problem hiding this comment.
There is only one number where that statement is false, so it seems a little misleading to put to the max value. Like why not make the value 0? Seems safer and hardcode the max uint check
|
|
||
| uint256 _liquidity0Full = type(uint256).max; | ||
| if (_totalAmount0Ref > 0) { | ||
| _liquidity0Full = |
There was a problem hiding this comment.
Why is 0 full and the other one called amount1?
There was a problem hiding this comment.
_liquidity0Full is the liquidity for position 0. By amount1 I'm guessing you might be referring to _liquidity0ByAmount1? That is how much of position 0 liquidity you can create if token1 is the limiting factor. I agree it's a bit confusing, open to name suggestions.
| } | ||
|
|
||
| /// @dev Calculates expected deposit outputs (shares = first position's liquidity). | ||
| function _calculateExpectedDeposit(uint256 _amount0, uint256 _amount1) |
There was a problem hiding this comment.
We shouldn't recreate the logic in the test. Then we aren't really testing anything. We should run the code and verify the invariants maintain correctness
| vm.startPrank(_depositor); | ||
| MockERC20(Currency.unwrap(currency0)).approve(address(vault), _amount0); | ||
| MockERC20(Currency.unwrap(currency1)).approve(address(vault), _amount1); | ||
| function _setupInitializer(address _addr) internal { |
There was a problem hiding this comment.
I am not a fan of this setup naming. To generic, and it doesn't describe what goes into the setup.
| LiquidityPosition[] public positions; | ||
|
|
||
| /// @notice Whether liquidity has been initialized. | ||
| bool public liquidityInitialized; |
There was a problem hiding this comment.
^ Make this immutable ?
There was a problem hiding this comment.
don't think so because liquidity doesn't get initialized in the constructor.
* Some deposit suggestions --------- Co-authored-by: garyghayrat <gary.ghayrat@pm.me>
garyghayrat
force-pushed
the
feat/change-deposit-tracking
branch
from
8dae55f to
f7db966
Compare
|
Coverage after merging feat/change-deposit-tracking into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
3 participants
closes #46