Yieltoken uni migration #154
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
pegahcarter
reviewed
Apr 28, 2022
pegahcarter
reviewed
Apr 28, 2022
pegahcarter
reviewed
Apr 28, 2022
pegahcarter
reviewed
Apr 28, 2022
| await weth.connect(account1).approve(migration.address, max); | ||
| await weth.connect(account0).approve(singleAssetVault.address, max); | ||
| await weth.connect(account1).approve(singleAssetVault.address, max); | ||
| await dai.connect(account1).approve(migration.address, max); |
There was a problem hiding this comment.
Why did we remove weth from this test?
There was a problem hiding this comment.
because here we are testing only curve changes the vault changes are tested in resubscribe multiple changes
pegahcarter
reviewed
Apr 28, 2022
pegahcarter
reviewed
Apr 28, 2022
| @@ -185,9 +185,10 @@ const setup = async () => { | |||
| expect(meTokenInfo.balanceLocked).to.equal(0); | |||
| }); | |||
| it("burn() [buyer]: assets received based on initial refundRatio", async () => { | |||
| const collateralDeposited = tokenDeposited.mul(3); | |||
There was a problem hiding this comment.
What is the motivation behind adding this multiplication?
pegahcarter
reviewed
Apr 28, 2022
pegahcarter
reviewed
Apr 28, 2022
| return ethers.utils.parseEther(num.toString()); | ||
| let res: BigNumber = BigNumber.from(0); | ||
| let curNum = num.toString(); | ||
| while (curNum.length > 1) { |
There was a problem hiding this comment.
What failing test case motivated this extra logic?
There was a problem hiding this comment.
this make the conversion more robust if you try to convert a number with too many decimals/number
pegahcarter
reviewed
Apr 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Let's say that you want to mint metokens but it is migrating from DAI to WETH before this PR the migration ends at the beginning of the mint whereas with this PR it happens at the end
Before this PR when migration ends if you want to mint for 1 Dai it will actually mint for 1 WETH. Therefore if you have no WETH it will revert but If you have one you actually mint for 1 WETH (roughly 3500 DAI now) instead of 1 DAI...
With this PR if you want to mint for 1 Dai it will actually mint for 1 DAI and it swaps all DAI for ETH and mark the migration as ended
also uniswap migration upgraded to SwapRouter V3