-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable pool fees when rebalancing Aerodrome AMO #2276
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2276 +/- ##
==========================================
- Coverage 53.26% 53.16% -0.11%
==========================================
Files 79 79
Lines 4089 4097 +8
Branches 1074 1074
==========================================
Hits 2178 2178
- Misses 1908 1916 +8
Partials 3 3 ☔ View full report in Codecov by Sentry. |
contracts/contracts/strategies/aerodrome/AerodromeAMOStrategy.sol
Outdated
Show resolved
Hide resolved
contracts/contracts/strategies/aerodrome/AerodromeAMOStrategy.sol
Outdated
Show resolved
Hide resolved
contracts/contracts/strategies/aerodrome/AerodromeAMOStrategy.sol
Outdated
Show resolved
Hide resolved
contracts/contracts/strategies/aerodrome/AerodromeAMOStrategy.sol
Outdated
Show resolved
Hide resolved
contracts/contracts/strategies/aerodrome/AerodromeAMOStrategy.sol
Outdated
Show resolved
Hide resolved
be5a5a5
to
946061a
Compare
@DanielVF & @shahthepro would you mind doing another pass? Aside from burning the OETHb left on the contract the main changes were a couple of fixes on the quoter contract (as described in PR description) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_checkForExpectedPoolPrice
should be view
not internal
I believe (though it's not part of this PR)
*/ | ||
if (tokenId != 0) { | ||
_removeLiquidity(1e18); | ||
_removeLiquidityToEnsureSwap(_amountToSwap, _swapWeth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can probably replace lines 431 to 439 with something like this. But not a big deal though since _removeLiquidityToEnsureSwap
has a amountToSwap > 0
check
if (_amountToSwap > 0) {
if (tokenId != 0) {
_removeLiquidityToEnsureSwap(_amountToSwap, _swapWeth);
}
_swapToDesiredPosition(_amountToSwap, _swapWeth, _minTokenReceived);
}
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think this would be cleaner.
contracts/contracts/strategies/aerodrome/AerodromeAMOStrategy.sol
Outdated
Show resolved
Hide resolved
contracts/contracts/strategies/aerodrome/AerodromeAMOStrategy.sol
Outdated
Show resolved
Hide resolved
* enhance the way we withdraw liquidity * better location * fix comment * update tests and simplify liquidity calculation * small fix
@@ -351,7 +351,7 @@ contract AerodromeAMOStrategy is InitializableAbstractStrategy { | |||
*/ | |||
function depositAll() external override onlyVault nonReentrant { | |||
uint256 _wethBalance = IERC20(WETH).balanceOf(address(this)); | |||
if (_wethBalance > 0) { | |||
if (_wethBalance > 1e12) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to do this > 1e12 thing inside _burnOethbOnTheContract()
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed it makes sense as a defensive measure: cf8084c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, though we might want to return early in the burn function if we just have dust. This way we'll only do non-dust burns.
Code Change Checklist
To be completed before internal review begins:
Internal review:
Deploy checklist
Two reviewers complete the following checklist: