Badger-Finance / strategy-convex-staking-optimizer

GNU Affero General Public License v3.0
10 stars 8 forks source link

feat: bveCVX as reward, bought through curve #6

Closed GalloDaSballo closed 2 years ago

GalloDaSballo commented 2 years ago
GalloDaSballo commented 2 years ago

Will look into the CI / CD failure

petrovska-petro commented 2 years ago

There is quite a bit of noise on the changes due to the prettier, but this swap section looks good, which is buying via curve meta pool, later transfers to gov fee and tree portions accordangly based on the performanceFeeGovernance

GalloDaSballo commented 2 years ago

Adding an extra harvest check on live migration test

Also removing extra variables that were unused as per @shuklaayush advice

sajanrajdev commented 2 years ago

Added support for the rest of the Convex setts configurations. Running the tests now will run each one of the test with the setup for each one of the 8 Convex setts individually. To run you can keep using the following:

brownie test

Additionally, since now the test suite takes 8 times longer to run, some of the tests may fail because by the time they are ran Infura will recognize the forked block as archived and will not fetch it for free accounts. For this reason, I changed the RPC provider to Alchemy.

As of this moment, all tests are passing for the 8 Setts, except for the tests involving more than one harvest for pBTC and sBTC. The reason for this is that their baseRewardsPool is expired or expiring in a matter of hours. Will add a check on the deploy function to reset the rewardsPool if needed.

GalloDaSballo commented 2 years ago

For the sake of security / protocol, we could proceed with the 6 setts that are passing the checks

GalloDaSballo commented 2 years ago

I think at this point is best to ask @dapp-whisperer for his review / what he's comfortable with moving forward with

GalloDaSballo commented 2 years ago

Fixed typo in test basic which was causing a bunch of errors

shuklaayush commented 2 years ago

except for the tests involving more than one harvest for pBTC and sBTC. The reason for this is that their baseRewardsPool is expired or expiring in a matter of hours. Will add a check on the deploy function to reset the rewardsPool if needed.

@sajanrajdev Can we manually call earmarkRewards(pid) on the Convex booster to fix this?

sajanrajdev commented 2 years ago

@shuklaayush, yes I was about to comment this. @GalloDaSballo, last time we encountered this situation with other Sett and we ran earmarkRewards(pid) to permissionlessly reset the rewards, we could do the same again in production or, at least in the meantime, in the fork environment while Convex calls it. I will integrate this call to the suite anyways so that the tests don't break in the future in case this happens again.

sajanrajdev commented 2 years ago

@GalloDaSballo I had to comment out the post-migration harvest on the migration live test while we deploy the new strategies because the current strategies being tested don't contain some of the parameters included on the resolver and the checks fail, will uncomment it once we deploy. I am also adding an earn and tend before the harvest.

sajanrajdev commented 2 years ago

@shuklaayush @GalloDaSballo, added a check to the deploy function and now it resets any pool that is expired or is expiring in the next 4 days. Tests for sBTC and pBTC are now passing with this change.

GalloDaSballo commented 2 years ago

Code LG

@dapp-whisperer want me to deploy and verify?

sajanrajdev commented 2 years ago

Ran all tests once more and found all of them to pass except for the test_single_user_harvest_flow and test_single_user_harvest_flow_remove_fees for pBTC and test_single_user_harvest_flow_remove_fees for Tricrypto2.

image

image

The failures were due to these strategies having shorter periods for their BaseRewardPools and, therefore, the pools expire as the test progress and they stop emitting before the last harvest executes. Added the BaseRewardsPool check and rest in the middle of the tests and now they pass. This confirms that errors in tests are only due to external environment factors. The contract works as expected and it LGTM.

image

image

image

Will deploy the logic contract.

sajanrajdev commented 2 years ago

New logic contract deployed and verified: https://etherscan.io/address/0xe73e74fab5e1cfe7545421d7dc63da42fc62b0d3#readContract

shuklaayush commented 2 years ago

For when we do the next upgrade:

sajanrajdev commented 2 years ago

For when we do the next upgrade:

  • cvxHarvsted --> cvxHarvested
  • Instead of storing the curve address provider in CurveSwapper, maybe we should just store the base and factory pool registry addresses. Should save some gas

Do you want to create an issue on this repo for it? In that way we will remember to do it.

sajanrajdev commented 2 years ago

Added deployment script: scripts/deploy_convex.py