-
Notifications
You must be signed in to change notification settings - Fork 44
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
Aura Strategies of Top 6 Pools (auraBAL, auraBAL Stable, wstETH, bbaUSD, rETH, COMP) #76
base: master
Are you sure you want to change the base?
Conversation
@CryptJS13 Thank you for the assistance, here is the first strategy for wstETH and wETH. |
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.
Hey, looks pretty good in general! Thanks for the great work! Some comments on minor stuff and some convention that we use. Let me know if you have any questions about anything :)
|
||
import "./base/AuraStrategyUL.sol"; | ||
|
||
contract AuraStrategystETH is AuraStrategyUL { |
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.
For the implemented strategy contracts, we prefer to fill out all the values within this contract instead of inputting them with the initialisation. As you can see in other strategy contracts in this repository, the only fields generally open for input in the initializeStrategy
function are _storage
and _vault
. All other values can be filled within this contract.
The reason is that there is additional room for error when we need to pass these values with the function call, and there is no reason for these values to change between development and testing and actual deployment.
With filling in these values also comes our naming convention, which would in this case be AuraStrategyMainnet_stETH
. Mainnet denoting that the implementation contains the values for Mainnet deployment. We don't really develop for any testnets, but we kept the convention this way anyways.
So please include all the values within this contract and let me know if you have any questions about this :)
address _depositReceipt; | ||
(_lpt,_depositReceipt,,,,) = IAuraBooster(booster).poolInfo(_auraPoolID); | ||
require(_lpt == underlying(), "Pool Info does not match underlying"); | ||
require(1 < _poolAssets.length && _poolAssets.length < 5, "_nTokens should be 2, 3 or 4"); |
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 don't think there's a need for this check with Balancer. We had it for Curve, as we did not have interface contracts added for other amounts of tokens in the pool. For Balancer I don't think it matters, as we don't need to import different interfaces.
// if the token is the rewardToken then there won't be a path defined because liquidation is not necessary, | ||
// but we still have to make sure that the toHodl part is executed. | ||
if (rewardBalance == 0 || | ||
(storedLiquidationDexes[token][rewardToken()].length < 1) && |
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.
If a certain stored variable, like rewardToken()
is called more than once within the execution scope, it is more gas efficient to pull it from storage to local memory only once and keep it local for the remainder of the scope. So in this case it would be most efficient to, at the start of this function put address _rewardToken = rewardToken()
and replace all other cases of rewardToken()
with _rewardToken
. Let me know if anything is unclear.
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.
And of course the same goes for any other variable that is stored and used in a similar way
function partialWithdrawalRewardPool(uint256 amount) | ||
internal | ||
{ | ||
IAuraBaseRewardPool(rewardPool()).withdraw(amount, false); //don't claim rewards at this point |
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 we use withdrawAndUnwrap
on the BaseRewardPool to make the withdrawal in a single step? That would also make the depositReceipt
variable obsolete.
{ | ||
uint256 stakedBalance = _rewardPoolBalance(); | ||
if (stakedBalance != 0) { | ||
IAuraBaseRewardPool(rewardPool()).withdrawAll(false); //don't claim rewards |
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.
Same here, but use withdrawAllAndUnwrap
{ | ||
uint256 stakedBalance = _rewardPoolBalance(); | ||
if (stakedBalance != 0) { | ||
IAuraBaseRewardPool(rewardPool()).withdrawAll(true); |
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.
Same, use withdrawAllAndUnwrap
uint256 public constant hodlRatioBase = 10000; | ||
address[] public poolAssets; | ||
address[] public rewardTokens; | ||
bytes32[] public dexes; |
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.
is this used?
@@ -0,0 +1,32 @@ | |||
{ |
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 pretty cool and useful :)
Quite some of the parameters won't be needed in the test as we add them to the strategy contract directly, but I think the config file is still useful. Thanks!
test/aura/stETH.js
Outdated
"underlying": underlying, | ||
"governance": governance, | ||
"liquidation": [{"balancer": [config.relatedTokens.aura, config.relatedTokens.wETH]}], | ||
"strategyArgs": strategyArgs |
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 input won't be needed at all when all the relevant parameters are added to the strategy contract.
test/aura/stETH.js
Outdated
"strategyArgs": strategyArgs | ||
}); | ||
|
||
await strategy.setSellFloor(0, {from:governance}); |
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.
Not needed, as the sellFloor is already 0 in the contract.
@CryptJS13 Thank you for your support and assistance, and sorry for the mess up here. I was trying to rename the branch with the pr. Here are the strategies for the top 6 pools. The AuraStrategyJoinPoolUL is the original UL base contract inherited by the stETH, rETH, COMP, auraBAL Stable pools. And bbaUSD pool requires using batchSwap to get the underlying token, so I created an AuraStrategyBatchSwapUL. At last, the single asset auraBAL staking with Aura using the AuraStrategyAuraBAL base contract. |
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.
A few comments. Really good work in general. Thanks!
_setDepositArrayIndex(_depositArrayPosition); | ||
_setBalancerPoolId(_balancerPoolID); | ||
_setDepositToken(_depositToken); | ||
setUint256(_HODL_RATIO_SLOT, 1000); |
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.
Seeing this now, not sure if the same problem is in the other bases, but we should set it with the value of _hodlRatio
, not hardcode the value
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.
Fixed. Thanks for the feedback! I saw this is the ConvexUL strategy, not sure if there is a reason that I am missing for the different implementation.
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.
Hey, found this too, must have been a mistake on my side when working on the ConvexUL strategy
address(this), | ||
request | ||
); | ||
|
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.
Would it be possible to implement something like this: https://github.com/harvest-finance/harvest-strategy/pull/71/files#diff-e96312f746bad02267da18daa9abef9d0f1b71c078c0888c02a4df5da415ba6bR254, here too. So check if we can swap the depositToken to the underlying for a better rate than depositing 1:1? Because almost always you get a 2-3% better result when buying auraBal in the market instead of depositing.
IAuraBaseRewardPool(stakePool).stakeAll(); // Stake | ||
} | ||
|
||
function addRewardToken(address _token) |
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 dont think this works. We can't add a reward token without also adding the liquidation info
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.
It should look more like this: https://github.com/harvest-finance/harvest-strategy/blob/master/contracts/strategies/convex/base/ConvexStrategy4Token.sol#L196
But then adding the path and dexes to storedLiquidationPaths
and storedLiquidationDexes
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.
Fixed! Thank you for the suggestions. Indeed the original implementation won't liquidate the new reward token successfully.
import "../../../base/interface/IVault.sol"; | ||
import "../../../base/interface/balancer/IBVault.sol"; | ||
|
||
contract AuraStrategyBatchSwapUL is |
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.
Some of the comments from AuraStrategyAuraBAL
are valid in this contract too.
bytes32 swapPoolId = balancerSwapPoolId(); | ||
|
||
// Try to swap more than deposit | ||
uint256 underlyingBefore = IERC20(underlyingToken).balanceOf(address(this)); |
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.
@CryptJS13 Hi Jasper, this is how I implement the swapping option instead of direct deposit. My idea is that if I use queryBatchSwap, there might be a risk of having a sandwich attack. So I swap it with a limit. And if the swap succeeds, we will receive more underlying, otherwise, the swap doesn't proceed, and we receive nothing. Then we go with the deposit way.
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 don't think there is a sandwich risk, as the quote and the swap happen within the same transaction. There is no way for someone to change the balances in the pool in between these actions.
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 have adjust it with querySwap before deposit. I was originally afraid if someone finds this transaction in the mempool, they can use the MEV bot to manipulate the price and thus affect the amount we get from the swap.
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.
Looking good! Left some small comments. Thanks for the work! :)
pragma solidity 0.5.16; | ||
pragma experimental ABIEncoderV2; | ||
|
||
import "hardhat/console.sol"; |
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 can be removed for production, from all other contracts too
bytes32 swapPoolId = balancerSwapPoolId(); | ||
|
||
// Try to swap more than deposit | ||
uint256 underlyingBefore = IERC20(underlyingToken).balanceOf(address(this)); |
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 don't think there is a sandwich risk, as the quote and the swap happen within the same transaction. There is no way for someone to change the balances in the pool in between these actions.
function addRewardToken( | ||
address _token, | ||
address[] memory _path2Reward, | ||
bytes32 _dexOption |
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.
The dexes could be inputed as a list too, to generalise for tokens that would need to be liquidated using multiple dexes. This is the implementation I made for an updated version of the Convex strategy: https://github.com/harvest-finance/harvest-strategy/pull/77/files#diff-e91096217bcc49a83d27481a3ff62b6f23f59ab99b9ebae1edf466798d38754eR192
pragma solidity 0.5.16; | ||
pragma experimental ABIEncoderV2; | ||
|
||
import "hardhat/console.sol"; |
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.
Last little thing :) Please remove
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.
Thank you for the reminder, just removed it.
Strategy for wstETH <> wETH pair with Aura.
During doHardWork(), all rewards (AURA and BAL) are liquidated to wETH, then deposited wETH into Balancer for Balancer Pool Token(BPT), and finally, deposit BPT back to Aura.
Before deploying the contract, the following steps are required.