Aerodrome OETH AMO #2019
Aerodrome OETH AMO #2019
Conversation
* Add Tooling for Base network * Fix scripts * Add empty migrations file * Fix failing fork tests
| * @param r2 The reserve of OETH. | ||
| * @return The calculated constant K. | ||
| */ | ||
| function getK(uint256 r1, uint256 r2) internal pure returns (uint256) { |
There was a problem hiding this comment.
Since this is an internal function, it should get an underscore.
|
|
||
| function _lpWithdraw(uint256 _lpTokensAmount) internal { | ||
| // Claim remaining rwards before withdrawing LP Tokens | ||
| aeroGaugeAddress.getReward(address(this)); |
There was a problem hiding this comment.
Do we lose these reward tokens if we don't withdraw them? If not, it might just make things easier to not withdraw them until they are ready to be passed on. Less gas, and less things that could go wrong during a withdraw.
| * the pool's balance is not worsened. | ||
| * Withdrawals are proportional so doesn't change the pools asset balance. | ||
| */ | ||
| modifier improvePoolBalance() { |
There was a problem hiding this comment.
This method should check the pool's balances before the action that the modifier wraps, then compare them again after.
This way we check that things got better during the action.
If I'm reading this correctly, this only compares the balances/ratios after the action has taken place, which means it will not be able to compare the before and after to see if the difference is better?
What is this modifier currently trying to do?
There was a problem hiding this comment.
What is this modifier currently trying to do?
This modifier is used to make sure after rebalancing, the pool ratio doesn't skew more than the allowed threshold. As long as the ratio remains within threshold, we should be good. It helps avoid errors from the strategist while rebalancing the pool. This also gives the strategist the flexibility to rebalance the pool as per the market conditions.
This way we check that things got better during the action.
To validate that we should define what we mean by "better" for the pool. Does it mean the pool should be closer to 50:50 ratio or should it be unbalanced till the safest point.
There was a problem hiding this comment.
"Better" is towards 50/50.
Going towards 50/50 is safer because the strategist can't trade away from 50/50 which is an action that guaranteed loses money.
Going towards 50/50 is also more flexible, in that the strategist may want to move only a little towards repegging things. This can keep the price still off peg, but be profitable for the strategy, and slow down redeems.
There was a problem hiding this comment.
Fixed here: 7933c0e
| * 10 digits. This way we move the decimal point by 36 places when doing the calculation | ||
| * and again by 36 places when we are done with it. | ||
| */ | ||
| uint256 k = (1e36 * IERC20(address(lpTokenAddress)).totalSupply()) / |
There was a problem hiding this comment.
Might want to avoid calling this 'k' because it's a very different thing than the AMM's k, which is also calculated elsewhere in the code.
| poolWETHBalance; | ||
| // prettier-ignore | ||
| // slither-disable-next-line divide-before-multiply | ||
| uint256 diff = (_wethAmount + 1) * k; |
There was a problem hiding this comment.
This calculation currently calculates a ratio, then uses that ratio to calculate the end result. This is slightly lossy in doing two calculations with rounding errors, which requires the extra high intermediate precision.
Would this be much simpler and less if we just move the (_wethAmount+1) to be directly multiplied by the totalSupply in the first step?
Something like:
(_wethAmount+1) * totalSupply / poolWETHBalance ?
There was a problem hiding this comment.
I tired using multiple values, it doesn't make any difference. so yea we can use the one you suggested, might save some gas too.
| Action | Value | Formula 1 | Formula 2 |
|---|---|---|---|
| withdraw | 220 | 82862338377910162699 | 82862338377910162699 |
| withdraw | 1 | 1035779229723877034 | 1035779229723877034 |
| withdraw | 0.0000000001 | 103577924 | 103577924 |
| withdrawAll | 300 | 30000000000000000001 | 30000000000000000001 |
| uint256 _amountIn, | ||
| uint256 _minAmountOut, | ||
| address _tokenIn, | ||
| address _recipient |
There was a problem hiding this comment.
The strategist should not be able to steal tokens from the strategy / vault. We should probably remove the _recipient option here.
There was a problem hiding this comment.
If we remove the _recipient, where should the swapped weth sent? Will it be held in the AMO contract?
…rotocol/origin-dollar into PraneshASP/aerodrome-amo
| * @param asset address of the asset | ||
| * @return uint256 unit price for 1 asset unit, in 18 decimal fixed | ||
| */ | ||
| function price(address asset) |
There was a problem hiding this comment.
this function is still identical to the one in OETHOracleRouter. You can create a OETHOracleRouterBase and implement the price function in it. And then have OETHOracleRouter and BaseOETHOracleRouter inherit from it
There was a problem hiding this comment.
Refactored here: 826361f
* feat: oethcore deployment * chore: fix deployment setup * chore: update global hook * config: update defi.yml * config: add base fork tests to defi.yml * Fix order of comparison * Minor tweaks * Fund correct deployer on fork * fix: tests * chore: fix tests * chore: remove stack trace flag * chore: fix typo * chore: fix test * fix: base fork test fixture * chore: rename deployment file * feat: OETHBase Vault base fork tests * fix: unit tests * feat: add more fork tests * feat: refactor deploy actions, fixture and fork test * chore: minor optimization * refactor: fix deploy actions and fixtures * refactor: fork tests * refactor: harvester contracts * refactor: tests and deploy action * chore: more refactoring * Update deploy scripts to use Guardian address (#2075) * wip * refactor: update deploy scripts to use guardian * chore: rename AeroHarvester -> OETHBaseHarvester * refactor: abstract oracle contract * chore: fmt * refactor: base harvester aerodrome route --------- Co-authored-by: Domen Grabec <grabec@gmail.com> Co-authored-by: Shahul Hameed <10547529+shahthepro@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2019 +/- ##
==========================================
- Coverage 60.34% 56.48% -3.87%
==========================================
Files 59 64 +5
Lines 3006 3247 +241
Branches 775 831 +56
==========================================
+ Hits 1814 1834 +20
- Misses 1189 1410 +221
Partials 3 3 ☔ View full report in Codecov by Sentry. |
Aerodrome OETH AMO
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
If you made a contract change, make sure to complete the checklist below before merging it in master.
Refer to our documentation for more details about contract security best practices.
Contract change checklist:
Notion resources: