Add initial variableDebtManager and HookAMM #91
Conversation
| } | ||
|
|
||
| function getVirtualDolaReserves() internal view returns(uint) { | ||
| return sqrt(1e18 * getK() / minDolaPrice); |
There was a problem hiding this comment.
Can you explain what's going on here? Why is minDolaPrice used here? and how does this square root result in dola reserves? Shouldn't we be dividing K by DBR reserve to get DOLA reserves?
| * @dev Calculates the DOLA reserve based on the current DBR reserve. | ||
| * @return The calculated DOLA reserve. | ||
| */ | ||
| function getDolaReserve() public view returns (uint) { |
There was a problem hiding this comment.
DBR reserve is not used here as claimed in the natspec comment. Can you explain how dola reserve should be calculated in the context of this AMM?
| } | ||
|
|
||
| function _invariantCheck(uint newDolaReserve, uint newDbrReserve, uint k) internal view { | ||
| if(newDolaReserve * newDbrReserve < k){ |
There was a problem hiding this comment.
Shouldn't it be > rather than <?
| uint maxDbr = k / maxDolaReserve; | ||
| uint excessDola = newDolaReserve - maxDolaReserve; | ||
| uint excessDbr = newDbrReserve - maxDbr; | ||
| if(1e18 * excessDbr / excessDola >= maxDolaPrice) revert Invariant(); |
There was a problem hiding this comment.
Why are we enforcing price invariants when the price is determined by our own AMM? This will essentially halt trading beyond price constraints while the intended behavior would be to continue trading at the price constraints. I think maybe the invariant design might be inappropriate here and instead we could opt for a simple price function that derives price from xy=k up until the constraints then returns fixed prices beyond them.
| if(exactDbrOut > dbrBal) | ||
| dbr.mint(address(this), exactDbrOut - dbrBal); | ||
| uint exactDbrOutAfterFee = exactDbrOut * (1e18 - dbrBuyFee) / 1e18; | ||
| feesAccrued += exactDbrOut - exactDbrOutAfterFee; |
There was a problem hiding this comment.
Why mint the fee at all? Instead we should deduct it from the minted amount (or burn it from balance) and remove the feeRecipient role entirely.
| if(totalDebt == 0){ | ||
| additionalDebtShares = additionalDebt * MANTISSA; //Minting a high amount of initial debt shares, as debt per share will increase exponentially over the lifetime of the contract | ||
| } else { | ||
| additionalDebtShares = additionalDebt * totalDebtShares / totalDebt; //TODO: Consider rounding up in favour of other users |
There was a problem hiding this comment.
We can simply add a + 1 instead of more complex rounding up, wdyt?
| } else { | ||
| uint removedDebtShares = totalDebtShares * amount / totalDebt; | ||
| totalDebt -= amount; | ||
| totalDebtShares -= removedDebtShares; //TODO: Make sure this doesn't underflow |
There was a problem hiding this comment.
It's impossible to underflow given the if condition above, no? It'll always be less than totalDebtShares
| totalDebt += dolaNeeded; | ||
| lastUpdate = block.timestamp; | ||
| dola.mint(address(this), dolaNeeded); | ||
| amm.burnDbr(dolaNeeded, _dbrDeficit); |
There was a problem hiding this comment.
Careful of the interface mismatch. Should be burnDBR
| uint dolaNeeded = helper.dolaNeededForDbr(_dbrDeficit); | ||
| totalDebt += dolaNeeded; | ||
| lastUpdate = block.timestamp; | ||
| dola.mint(address(this), dolaNeeded); |
There was a problem hiding this comment.
There's no approval to the AMM, how is it expected to pull the DOLAs from this contract?
Also I'm not sure debt manager's should each have DOLA minting rights. The risk seems disproprotionately high. Either we should pre-deposit DOLAs in it or there should be an intermediate middleware than restricts infinite minting. I prefer the first option imo
| } | ||
| } | ||
|
|
||
| function dbrDeficit() public view returns (uint){ |
There was a problem hiding this comment.
In the market contract on the #56 branch, the interface of this function has an address borrower param. Seems to mismatch here.
Variable rate market