Refactor the tests from Kevins PR #54 to be more modular and robust
Make fixes that were missed
Core changes:
Test Refactor
I refactored the tests to test the entire deposit and redeem tests on their own, like a unit test. These are down in the private functions that start with _
Then for functions like testRedeemStablecoinDAI() we run an integration test where both _testDepositAnyModule() and _testRedeemAnyModule() are run together. In my experience at The Graph, this ended up being the cleanest and most repeatable way to write safe code, so let's do it like this as often as possible (when there are a lot of before and after checks. The struct Before and struct After I had to create because of stack too deep errors, but they actually cleaned up the code really nicely.
This closes out the main comments around the tests
Other fixes
Please see This commit I made for the small fixes. A few were missed by you from my previous PR review, and some were small fixes I made
Missed:
Deleting the events StablecoinWhitelisted and StabecoinDelisted. I mentioned this in my previous PR review and you resolved it, but they were still there. Please make sure to ask any questions if my comments are unclear
vm.stopPrank() could be a few lines lower to catch the last approve
Updated by me
Rename OverEighteenDecimalPlaces() to just OverEighteenDecimals()
moved the revert for this to the constructor, instead of in every deposit and withdraw, to
Other notes
I just noticed you have been doing branches as feat/NAME_OF_BRANCH, but our standard is to do YOUR_NAME/NAME_OF_BRANCH - so please do that going forwards. Thanks!
In the first commit I also refactored us to use IERC20Metadata instead of ERC20. saves on gas deployment due to it being much less code. IT also allowed me to more easily call the stablecoin balances while doing the refactor of the tests
STEPS TO MERGE
Kevin, please give this a review. If it all looks good to you, merge into your branch, or give me comments and I will fix.
Then lets squash your branch into main, there are already 13 commits and its getting pretty messy, I think the squash will be fastest.
Primary Goals of PR
Core changes:
Test Refactor
_
testRedeemStablecoinDAI()
we run an integration test where both_testDepositAnyModule()
and_testRedeemAnyModule()
are run together. In my experience at The Graph, this ended up being the cleanest and most repeatable way to write safe code, so let's do it like this as often as possible (when there are a lot of before and after checks. Thestruct Before
andstruct After
I had to create because of stack too deep errors, but they actually cleaned up the code really nicely.Other fixes
StablecoinWhitelisted
andStabecoinDelisted
. I mentioned this in my previous PR review and you resolved it, but they were still there. Please make sure to ask any questions if my comments are unclearvm.stopPrank()
could be a few lines lower to catch the last approveOverEighteenDecimalPlaces()
to justOverEighteenDecimals()
Other notes
feat/NAME_OF_BRANCH
, but our standard is to doYOUR_NAME/NAME_OF_BRANCH
- so please do that going forwards. Thanks!IERC20Metadata
instead ofERC20
. saves on gas deployment due to it being much less code. IT also allowed me to more easily call the stablecoin balances while doing the refactor of the testsSTEPS TO MERGE