Closed AliyaDav closed 1 year ago
I am still reviewing the contract and tests, but the merging seems to be done incorrectly.
Most importantly:
- CurveConvexStrategyV2.sol and CurveConvexStrategyV2Native.sol should be in contracts/ not contracts/deprecated as they are currently in prod
- The existing test files for the two V2 strategies have been overwritten and removed
test/CurveConvexStrategyV2.ts test/CurveConvexStrategyV2Native.ts
You should rename your test file and then return these two files.
The best approach is to use git rebase to resolve it. Use the current master [ab8df9c] as the root inside your branch and tack on your commits.
Here is a guide that might help: https://docs.gitlab.com/ee/topics/git/git_rebase.html
Additional notes:
I am thinking more generally about using the stored kek_ids mapping vs interacting directly with FraxConvex by calling lockedStakedOf.
In invest you directly call the fraxConvex method, however in withdraw you use the stored mapping value. Because these methods are not consistent, I am wondering if there are some edge cases.
Can you explain the difference between lockedStakesOf vs lockedLiquidityOf and whether the liquidity object in lockedStakes.liquidity is equal to the return value from lockedLiquidityOf?
Please write some additional test cases with partial withdrawals from frax convex strategies. In particular, I believe that this contract does not work if we decide to (in this sequence):
a. Withdraw partially from a frax pool (say, 60%)
b. Deposit that 60% into another strategy, but then lock the existing 40% stakes longer.
In fact, it looks like the strategy just leaves the 40% staked but not locked, therefore losing out on all FXS rewards.
Additional Comments Pt.2:
Has some bugs I believe:
a. Line229: claiming rewards without checking the bool shouldWithdrawRewards in callData.
b. the partial withdrawals then locking of remaining lps as mentioned above
Can we have some unit test cases for the getDeployedAmount + getDeployedAmount and rewards functions?
Invest() function may have some edge cases that I am thinking about.
Are we sure we want to take the whole LP balance? It will work fine in the context of the vote executor system, but the function will actually not work as described.
eg. lock 100k usdc worth of lps --> ends up being 300k usdc worth of lps if we had 200k worth of fraxConvex lps sitting in the contract.
This is linked to whether we have the remaining funds locked in the contract or not in the exitAll function so you might think of a joint solution there.
Comments Pt.1:
lockedStakesOf().kek_id
instead.lockedStakesOf(adress).liquidity
and lockedLiquidityOf
return the same value.
I'm using lockedStakesOf
in invest()
since I need both to see if the length of the returned array is > 0 and I need kek_id field of the tuple returned, whereas in getDeployedAmount
ana getDeployedAmountAndRewards
I only need the amount of liquidity locked. _lockRemaining()
which automatically locks the remaining LP tokens which haven't been unwrapped. The duration of locking is the minimum locking period of the corresponding frax pool.
I also added lockRemaining()
in case we'll need to manually lock LP tokens remaining on the strategy contract. Additional Comments Pt.2:
a. Moved claiming rewards into if(shouldWithdrawRewards)
b. Fixed as described above.
There are already tests for that: "Should return LP position in fiat" and "Should return LP position in fiat and claim rewards" in CurveFraxConvexStrategyV2.ts
.
TBD.
Some updates:
Now when invest(data, amount) is called, we enter the pool with the exact amount passed in params. So, if there were crvLpTokens or wrapped crvLpTokens in the contract before, those would not be moved. This case is tested in CurveFraxConvexStrategyV2.ts
"Should invest exactly the amount passed in the params, regardless of previous contract balance"
unit test.
Data passed to ExitAll() now has one more encoded param - bool lockRemaining. The test for locking remaining and not locking are covered in "Should exit only 60% and lock remaining LP tokens for longer"
and "Should exit only 60% without locking remaining LP tokens for longer"
P.S. To clarify, when we call exitAll()
we will exit from curve exactly with the amount that was unlocked from frax. If, by any chance, there were other crvLpTokens in the contract before exitAll()
, those will not be moved.
In general, no extra crvLpTokens are expected to be in the contract, since those which fall into the scope of operations defined by the contract logic are expected to be moved only inside invest()
and exitAll()
Below is the logic of exitAll()
On the contract level:
Since we are only using this in the exitAll, can we pass an extra data uint of duration like we do in the invest function? This is because I want us to remain flexible with the implmenetation we have, otherwise we will need to upgrade everytime.
Can you just calculate teh withdraw amoutnt then just lock the rest of the balance? Otherwise I believe we will have dust remaining, which is just a bit annoying.
These are contract side so I believe the tests will be affected.
I am reviewing the test cases + comments above to make sure we have covered all bases.
Tests failing
1) Automated strategy execution
Testing FRAX strategies with ERC20 pool tokens
Should add data to strategy handler :
Error: missing argument: passed to contract (count=4, expectedCount=5, code=MISSING_ARGUMENT, version=contracts/5.7.0)
at Logger.makeError (node_modules\@ethersproject\logger\src.ts\index.ts:269:28)
at Logger.throwError (node_modules\@ethersproject\logger\src.ts\index.ts:281:20)
at Logger.checkArgumentCount (node_modules\@ethersproject\logger\src.ts\index.ts:340:18)
at E:\Coding Projects\alluo-strategies\node_modules\@ethersproject\contracts\src.ts\index.ts:187:12
at step (node_modules\@ethersproject\contracts\lib\index.js:48:23)
at Object.next (node_modules\@ethersproject\contracts\lib\index.js:29:53)
at E:\Coding Projects\alluo-strategies\node_modules\@ethersproject\contracts\lib\index.js:23:71
at new Promise (
2) Automated strategy execution
Testing FRAX strategies with ERC20 pool tokens
Should submit, approve and execute liquidity direction:
Error: missing argument: passed to contract (count=4, expectedCount=5, code=MISSING_ARGUMENT, version=contracts/5.7.0)
at Logger.makeError (node_modules\@ethersproject\logger\src.ts\index.ts:269:28)
at Logger.throwError (node_modules\@ethersproject\logger\src.ts\index.ts:281:20)
at Logger.checkArgumentCount (node_modules\@ethersproject\logger\src.ts\index.ts:340:18)
at E:\Coding Projects\alluo-strategies\node_modules\@ethersproject\contracts\src.ts\index.ts:187:12
at step (node_modules\@ethersproject\contracts\lib\index.js:48:23)
at Object.next (node_modules\@ethersproject\contracts\lib\index.js:29:53)
at E:\Coding Projects\alluo-strategies\node_modules\@ethersproject\contracts\lib\index.js:23:71
at new Promise (<anonymous>)
at __awaiter (node_modules\@ethersproject\contracts\lib\index.js:19:12)
at populateTransaction (node_modules\@ethersproject\contracts\lib\index.js:152:12)
3) Automated strategy execution
Testing FRAX strategies with ERC20 pool tokens
Should try exiting before the end of locking period:
Error: missing argument: passed to contract (count=4, expectedCount=5, code=MISSING_ARGUMENT, version=contracts/5.7.0)
at Logger.makeError (node_modules\@ethersproject\logger\src.ts\index.ts:269:28)
at Logger.throwError (node_modules\@ethersproject\logger\src.ts\index.ts:281:20)
at Logger.checkArgumentCount (node_modules\@ethersproject\logger\src.ts\index.ts:340:18)
at E:\Coding Projects\alluo-strategies\node_modules\@ethersproject\contracts\src.ts\index.ts:187:12
at step (node_modules\@ethersproject\contracts\lib\index.js:48:23)
at Object.next (node_modules\@ethersproject\contracts\lib\index.js:29:53)
at E:\Coding Projects\alluo-strategies\node_modules\@ethersproject\contracts\lib\index.js:23:71
at new Promise (<anonymous>)
at __awaiter (node_modules\@ethersproject\contracts\lib\index.js:19:12)
at populateTransaction (node_modules\@ethersproject\contracts\lib\index.js:152:12)
4) Automated strategy execution
Testing strategies with native ETH
Should invest into ETH/frxETH pool:
Error: missing argument: passed to contract (count=4, expectedCount=5, code=MISSING_ARGUMENT, version=contracts/5.7.0)
at Logger.makeError (node_modules\@ethersproject\logger\src.ts\index.ts:269:28)
at Logger.throwError (node_modules\@ethersproject\logger\src.ts\index.ts:281:20)
at Logger.checkArgumentCount (node_modules\@ethersproject\logger\src.ts\index.ts:340:18)
at E:\Coding Projects\alluo-strategies\node_modules\@ethersproject\contracts\src.ts\index.ts:187:12
at step (node_modules\@ethersproject\contracts\lib\index.js:48:23)
at Object.next (node_modules\@ethersproject\contracts\lib\index.js:29:53)
at E:\Coding Projects\alluo-strategies\node_modules\@ethersproject\contracts\lib\index.js:23:71
at new Promise (<anonymous>)
at __awaiter (node_modules\@ethersproject\contracts\lib\index.js:19:12)
at populateTransaction (node_modules\@ethersproject\contracts\lib\index.js:152:12)
For future reference:
We discussed this and it turns out husky git hooks are not immediately configured correctly when pulling from git. It would've stopped the push completely as tests were failing but since the hooks didn't automatically run the test, it was bypassed.
chmod ug+x .husky/pre-commit chmod ug+x .husky/pre-push
Solved the issues
Two things:
Other than that, I think https://github.com/GetAlluo/investment-strategies/pull/25/commits/10fee4060bcd11926ccf1bec3ad53be67bf4e665 is ready for deployment. Pls prepare deployment steps
For adding FRAX/USDC strategy:
call strategy handler (0x385AB598E7DBF09951ba097741d2Fa573bDe94A5)
addLiquidityDirection(
"Curve/FraxConvex Frax+USDC",
"0x7f609E0b083d9E1Edf0f3EfD1C6bdd2b16080EEd",
"0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48", // entry token USDC
0, // assetId - USDC
1, // chain id
"0x000000000000000000000000dcef968d416a41cdac0ed8702fac8128a64241a2000000000000000000000000a0b86991c6218b36c1d19d4a2e9eb0ce3606eb4800000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000001000000000000000000000000963f487796d54d2f27ba6f3fbe91154ca103b19900000000000000000000000000000000000000000000000000000000000a8c00", // entry data
"0x000000000000000000000000dcef968d416a41cdac0ed8702fac8128a64241a2000000000000000000000000a0b86991c6218b36c1d19d4a2e9eb0ce3606eb480000000000000000000000000000000000000000000000000000000000000001000000000000000000000000963f487796d54d2f27ba6f3fbe91154ca103b199000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000a8c00", // exit data
"0x0000000000000000000000003175df0976dfa876431c2e9ee6bc45b65d3473cc000000000000000000000000963f487796d54d2f27ba6f3fbe91154ca103b1990000000000000000000000000000000000000000000000000000000000000000" // rewards data
)
For adding ETH/frxETH strategy:
call strategy handler (0x385AB598E7DBF09951ba097741d2Fa573bDe94A5)
addLiquidityDirection(
"Curve/FraxConvex ETH+frxETH",
"0x4d8dE98F908748b91801d74d3F784389107F51d7",
"0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", // entry token
2, // assetId - WETH
1, // chain id
"0x000000000000000000000000a1f8a6807c402e4a15ef4eba36528a3fed24e577000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc200000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a537d64881b84faffb9ae43c951eebf368b71cda00000000000000000000000000000000000000000000000000000000000a8c00", // entry data
"0x000000000000000000000000a1f8a6807c402e4a15ef4eba36528a3fed24e577000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a537d64881b84faffb9ae43c951eebf368b71cda000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000a8c00", // exit data
"0x000000000000000000000000f43211935c781d5ca1a41d2041f397b8a7366c7a000000000000000000000000a537d64881b84faffb9ae43c951eebf368b71cda0000000000000000000000000000000000000000000000000000000000000000" // rewards data
)
Related to #20
CurveFraxConvexStrategyV2.sol
in contracts is the contract for both native ETH and ERC20 tokens to invest in Frax Convex Finance.CurveConvexStrategyNativeV2.ts
in tests is forking mainnet and runs unit tests.IntegratedFraxConvex.ts
in tests is forking mainnet and runs integration tests.