-
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
Add Inverted Quoter for AMO fork test. #2210
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2210 +/- ##
==========================================
- Coverage 56.32% 54.22% -2.10%
==========================================
Files 77 78 +1
Lines 3867 4009 +142
Branches 767 1047 +280
==========================================
- Hits 2178 2174 -4
- Misses 1686 1832 +146
Partials 3 3 ☔ View full report in Codecov by Sentry. |
Add Inverted Quoter for AMO fork test.
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
const currentPrice = await aerodromeAmoStrategy.getPoolX96Price(); | ||
const priceTick0 = await aerodromeAmoStrategy.sqrtRatioX96TickLower(); | ||
const priceTick1 = await aerodromeAmoStrategy.sqrtRatioX96TickHigher(); | ||
const targetPrice = priceTick0 * 0.2 + priceTick1 * 0.8; |
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: these lines of code repeat a couple of times. You could make a helper function that only returns the amount to swap and direction?
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.
Also could the AMO quoter get reference to the pool and figure out the current price and the direction of the swap by itself?
contract AerodromeAMOQuoter { | ||
QuoterHelper public quoterHelper; | ||
|
||
constructor() { |
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.
constructor should probably also contain the amoStrategy
as a parameter and clPool
(underlying aerodrome slipstream pool)
I left a couple of comments, but generally I would adjust the api between the fork tests and the quoter a little. The code that is issuing the call from the fork test doesn't really need to pass any variables to the quoter. The Quoter should be constructed using the pool & strategy contract and be able to figure out the direction and the amounts to swap on its own. Separately from that some of the fork tests also also try to move the price in the pool close to left tick boundary or right tick boundary. Do you think you could also write a helper functions so we don't have those swap values hardcoded and needing to adjust them frequently? |
const value = transferEvent.args.value; | ||
await oethbVault | ||
.connect(await impersonateAndFund(addresses.base.governor)) | ||
.setStrategistAddr(addresses.base.strategist); |
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.
I think it would be better to read the active strategist address from the vault await oethbVault.strategistAddr()
before changing it. And whatever the first function returns set it here. Just so that you return things to their initial state whatever that is.
Ok got it, no problem it makes sense.
I had already built a quoter that gave the quantity to be exchanged to reach a certain price, I can integrate it without any problem, sure ;) |
I've updated some tests with the quoter that returns the amount to swap to reach a certain price. Let me know if you want me to apply it to other tests that I might have missed. 😉 |
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.
Did a pass please take a look
uint160 sqrtPriceCurrentX96, | ||
uint160 sqrtPriceTargetX96 | ||
) public pure returns (bool) { | ||
uint256 allowedVariance = (sqrtPriceTargetX96 * |
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.
I am not sure this is the correct method to calculate variance. Can you share your thoughts on how you got to this? Here I am comparing the way I approached variance and the result this function returns. The results differ in 3 orders of magnitude:
Tick -1 x96: 79224201403219477170569942574
Tick 0 x96: 79228162514264337593543950336
Diff X96: [tick -1 x96] - [tick 0 x96]
Diff X96: 3961111044860422974007762
variance 1% : Diff / 100
variance method 1: 39611110448604229740078
// solidity shell
uint256 PERCENTAGE_BASE = 1e27;
uint256 ALLOWED_VARIANCE_PERCENTAGE = 1e18;
uint160 sqrtPriceTargetX96 = 79224201403219477170569942574;
uint256 allowedVariance = (sqrtPriceTargetX96 * ALLOWED_VARIANCE_PERCENTAGE) / PERCENTAGE_BASE;
variance method 2: 79224201403219477170
variance method 1: 39611110448604229740078
variance method 2: 79224201403219477170
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.
I'm not sure I really understand your point as I don't think we are comparing the same thing at the end.
On my side I'm calculating the "variance" around the target price, while you calculate the "variance" as the difference between two ticks. That's why there is a that huge difference I will say.
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.
I see, I guess I am interested in the logic behind how the variance is calculated. In other words why the approach to calculate variance from the total x96 format number.
For example my logic behind why to specify variance in a way where the difference between to ticks represented 100% of the variance is:
- we deposit our whole liquidity in the [-1,0] ticker
- when we calculate the WETH to OETHb share of the liquidity the 1 tick spacing (the width of the ticker) represents 100% of the liquidity range
- meaning that 5% in the price variance calculation will also represent 5% of the WETH liquidity share in the ticker range.
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.
Ok I see, no problem, I've updated it. Let me know if you prefer it like this.
/// --- FUNCTIONS | ||
//////////////////////////////////////////////////////////////// | ||
/// @notice This call can only end with a revert. | ||
function getAmountToSwapBeforeRebalance() public { |
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.
This is cool, would be really useful to also have a function getAmountToSwapBeforeRebalance(uint256 overrideBottomWethShare, uint256 overrideTopWethShare)
. And the function should just issue a temporary change to the allowed WETH share interval.
The reason it is useful to have this function is to be able to test for failure in our fork tests when say, the rebalance ends up in having 10% above or below the desired WETH share amounts. Currently we hardcode those amounts in tests and I am afraid we will need to adjusts the tests constantly when other LP providers enter the pool
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.
Indeed that's a great idea! I'm doing it 😉
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.
Done, let me know what you think of it.
/// @param overrideTopWethShare New value for the allowedWethShareEnd on AMO. | ||
/// Use type(uint256).max to keep same value. | ||
/// @return data Data struct with the amount and the number of iterations | ||
function quoteAmountToSwapBeforeRebalance( |
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.
awesome stuff!
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.
Thanks!
@@ -46,6 +48,12 @@ describe("ForkTest: Aerodrome AMO Strategy empty pool setup (Base)", function () | |||
await oethb | |||
.connect(rafael) | |||
.approve(aeroSwapRouter.address, oethUnits("1000")); | |||
|
|||
await deployWithConfirmation("AerodromeAMOQuoter", [ |
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.
this will fit better in here:
const { isFork, isBaseFork, oethUnits } = require("./helpers"); |
import { IQuoterV2 } from "../interfaces/aerodrome/IQuoterV2.sol"; | ||
import { IAMOStrategy } from "../interfaces/aerodrome/IAMOStrategy.sol"; | ||
|
||
contract QuoterHelper { |
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.
can you add a small comment here why the QuoterHelper is required. It might not be immediately obvious to others that try/catch in solidity requires a separate contract in order to function
if (sqrtPriceCurrentX96 > sqrtPriceTargetX96) { | ||
return sqrtPriceCurrentX96 - sqrtPriceTargetX96 <= allowedVariance; | ||
return (sqrtPriceCurrentX96 - sqrtPriceTargetX96) * PERCENTAGE_BASE <= ALLOWED_VARIANCE_PERCENTAGE * range; |
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.
cool that makes sense.
nit: Maybe a slightly more "natural" way to express it would be:
return (sqrtPriceCurrentX96 - sqrtPriceTargetX96) <= range * ALLOWED_VARIANCE_PERCENTAGE / PERCENTAGE_BASE;
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 agree, I was to avoid rounding issue. But I've no problem to change it.
true, // _swapWETH | ||
oethUnits("0.0009") | ||
) | ||
rebalance(value.mul("200").div("100"), direction, 0) |
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.
we can change this by fetching the swap amount by setting the price of the say tick 1 and tick -2 from the quoter
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.
I'm sorry, I didn't get what do you mean. You could explain it another way please?
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.
Sorry, for a bad initial comment :)
So what this test is doing is trying to trigger an error where too much funds would be swapped and the result would be a price out of tick. The way it is currently done is getting amount to swap to reach an ok target balance and multiplying the value by 2. While that might work right now I am afraid it doesn't make the test super robust, since there are probably base mainnet values where where this test won't trigger a fail.
Now ignore my initial comment.
I think the goal should be to get values in the test that will be always out of bounds no matter what the state of the mainnet base is.
You could probably achieve that by calling: quoteAmountToSwapBeforeRebalance(0.98e18, 1e18)
and then multiplying that value by 2. Of if you can think of a more robust method yourself might be even better?
Ideally we would set a price out of bounds. Say they price of the pool at ticker 1 (which is out of bounds) and get the value we need to swap for to reach that ticker when rebalancing.
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.
Thanks!
I've applied some changes, let me know what do you think of it 😉
You could probably achieve that by calling: quoteAmountToSwapBeforeRebalance(0.98e18, 1e18) and then multiplying that value by 2.
I didn't multiplied it by 2, I just used the value obtained, otherwise I'll have push the price outside of bounds.
I used 0.90/0.92 for share instead of 0.98/1 due to this.
require(_allowedWethShareEnd < 0.95 ether, "Invalid interval end"); |
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.
Those changes look cool thanks, good thinking on how to bring the swap liquidity to the edge of the tick and not go over.
My previous comment would actually apply better for testing this test Should be reverted trying to rebalance and we are not in the correct tick
that is failing on fork tests. Which should trigger when the rebalance crosses the tick boundary.
Would you mind adding the tick boundary tests for both directions please (crossing -1 tick going into -2 one. And crossing 0 tick going into tick 1). There are also a couple of other tests that are failing.
But I think the approach is solid now 💪
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.
I've fixed the failing test (due to the new timelock mostly).
I've preferred to push the price at tick -2 and then try to rebalance without swapping to force reverting on OutsideExpectedTickRange
instead of PoolRebalanceOutOfBounds
that will occur if I were modifying WethShareStart/End.
So there are now two tests:
- when the pool is at tick -2 and we try to rebalance with
swappingAmount == 0
. - when the pool is at tick 1 and we try to rebalance with
swappingAmount == 0
.
Let me know if this is ok for you, or if you prefer another approach 😉
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.
nice that is also a cool approach without needing to drastically change the functionality
@@ -723,7 +785,11 @@ describe("ForkTest: Aerodrome AMO Strategy (Base)", function () { | |||
}); | |||
|
|||
await expect( | |||
rebalance(oethUnits("4.99"), true, oethUnits("4")) | |||
rebalance( | |||
(await weth.balanceOf(await aerodromeAmoStrategy.clPool())).mul("2"), |
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.
nice fix
@clement-ux did another pass |
function _quoteAmountToSwapBeforeRebalance( | ||
uint256 overrideBottomWethShare, | ||
uint256 overrideTopWethShare | ||
) public returns (Data memory data) { |
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: was this supposed to be internal?
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.
Indeed, fixed!
expect(await weth.balanceOf(aerodromeAmoStrategy.address)).to.lte( | ||
BigNumber.from("1000") | ||
BigNumber.from("1000000") |
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.
@clement-ux can you add comments to these tests specifying the math/logic around why these are so high?
|
||
await expect( | ||
aerodromeAmoStrategy | ||
.connect(strategist) | ||
.rebalance(oethUnits("0"), false, oethUnits("0")) | ||
.rebalance(oethUnits("0"), direction, oethUnits("0")) | ||
).to.be.revertedWith("OutsideExpectedTickRange"); |
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: you can also check for the tick value here now that the error contains it
Couple of minor comments and a merge conflict. Then I am happy to do a final pass. Good job on this one |
3c85d1a
to
10e1ba8
Compare
…for-AMO-fork-tests
* remove unneeded var * fix unit tests * fix some slither stuff * fix fork test * fix bug when swap amount too small. fix bug when no liquidity in AMO pool to reach tick * remove not needed quoter functions. Correct the tests * remove * return * prettier * add ability to chai to parse custom errors * de-nest test file * correct min values
…for-AMO-fork-tests
Description
Calculating the amount of tokens to swap for rebalancing is not easy and always changes in the fork test, as the pool situation changes too.
Using a quoter that performs a binary search to find the amount to swap, to push the price in the correct range in the AMO, will contribute to the stability of the fork test.
Code Change Checklist
This PR doesn't change the code, it only changes the test.