Arbitrum Curve Finance Two Pool Strat. All tests passing. Ready for review.#2
Arbitrum Curve Finance Two Pool Strat. All tests passing. Ready for review.#2pata-eth wants to merge 3 commits intoarbitrum-tricryptofrom
Conversation
pata-eth
left a comment
pata-eth
left a comment
There was a problem hiding this comment.
@poolpitako, please review the changes after the feedback received. There's also a change related to the migration of the gauge which was to blame for the strategy not making money. The problem with the new gauge is that it holds no CRV, so the strategy is still not making money in the tests. All tests are now passing. Thank you
| name: StrategyCurveTwoPool | ||
|
|
||
| dotenv: .env | ||
|
|
There was a problem hiding this comment.
.env wasn't being loaded without this config
| # contracts used by the strategy | ||
| registry: "0x3199437193625DCcD6F9C9e98BDf93582200Eb1f" | ||
| gauge: "0xbF7E49483881C76487b0989CD7d9A8239B20CA41" | ||
| gauge: "0xCE5F24B7A95e9cBa7df4B54E911B4A3Dc8CDAf6f" |
There was a problem hiding this comment.
gauge migrated to a new address => https://gov.curve.fi/t/sidechain-gauge-upgrade-and-migration/3869
scripts/utils.py
Outdated
| Want: {strategy.balanceOfWant()} ({want.symbol()}) | ||
| Target Token: {target.balanceOf(strategy)} ({target.symbol()}) | ||
| CRV: {crv.balanceOf(strategy)} ({crv.symbol()}) | ||
| Claimable CRV: {gauge.claimable_reward.call(strategy, crv,{"from": |
There was a problem hiding this comment.
added entry to track CRV claimable by strategy
scripts/utils.py
Outdated
| Claimable CRV: {gauge.claimable_reward.call(strategy, crv,{"from": | ||
| strategy})} ({crv.symbol()}) | ||
| Gauge Token: {strategy.stakedBalance()} ({gaugeToken.symbol()}) | ||
| Gauge CRV Balance: {crv.balanceOf(gaugeToken)} ({crv.symbol()}) |
There was a problem hiding this comment.
added entry to track CRV available in the gauge
| ) | ||
| else: | ||
| assert new_params["totalGain"] - prev_params["totalGain"] > donation | ||
| assert new_params["totalGain"] - prev_params["totalGain"] >= donation |
There was a problem hiding this comment.
changed to '>=' to include the scenario in which the gauge has no CRV to distribute
tests/test_simple_harvest.py
Outdated
| gaugeCrvAmount = rewardToken.balanceOf(gauge) | ||
|
|
||
| if gaugeCrvAmount == 0: | ||
| warn("No CRV in the gauge that can be claimed by gauge token holders.") |
There was a problem hiding this comment.
if there's no CRV to distribute, we warn the user
tests/test_simple_harvest.py
Outdated
| warn("No CRV in the gauge that can be claimed by gauge token holders.") | ||
| else: | ||
| # If there's CRV in the gauge, then we must have a non-zero claim | ||
| stratClaimableCrv = gauge.claimable_reward.call(strategy, rewardToken) |
There was a problem hiding this comment.
if there's CRV available in the gauge, the strategy must have a non-zero value to claim
tests/test_simple_harvest.py
Outdated
| # confirm we made money | ||
| assert new_assets > old_assets | ||
| # confirm we made money, or at least that we have about the same | ||
| assert new_assets >= old_assets |
There was a problem hiding this comment.
changed to '>=' to include the scenario in which the gauge has no CRV to distribute
pata-eth
force-pushed
the
arbitrum-tricrypto
branch
from
03d9ecd to
ee9c321
Compare
|
@poolpitako, could you please review and approve this PR instead? ===> dudesahn#6 |
pata-eth
force-pushed
the
arbitrum-tricrypto
branch
from
ee9c321 to
5a67538
Compare
|
review moved to the following PR: dudesahn#6 |
2 participants
Created a PR against the arbitrum-tricrypto branch as that was the root branch for arbitrum-2crv. @poolpitako, this is ready for your review. At the strat meeting, folks mentioned that we'll need another dev to peer review the code. Ideally, that'd be dude. Note that this PR is only created to facilitate the code review and is not meant to be merged.