Conversation
NouDaimon
left a comment
NouDaimon
left a comment
There was a problem hiding this comment.
I only reviewed the contracts. Upgradeability is a must.
| import { DataTypes as SwapTypes } from "../optyfi-swapper/contracts/swap/DataTypes.sol"; | ||
| import { DataTypes } from "./DataTypes.sol"; | ||
|
|
||
| interface IVault { |
There was a problem hiding this comment.
I just realized this can simply be imported from earn-protocol and should simply be imported as opposed to recreated.
| * @param _spender address of the spender | ||
| * @param _amount transfer amount | ||
| */ | ||
| function _approveTokenIfNeeded( |
There was a problem hiding this comment.
I do not think it is good practice to approve for uint256.max because if a vulnerability is found which can exploit this, the approval is a big issue.
In this context it may not matter, although I would avoid doing it even here.
@dhruvinparikh @mariogutval thoughts?
There was a problem hiding this comment.
The idea is to reduce gas consumption. As the Zap contract doesn't hold tokens, I think it's not a problem to give max allowance.
NouDaimon
left a comment
NouDaimon
left a comment
There was a problem hiding this comment.
Readability in test cases is paramount - this is the point where other developers can ensure code functions correctly. It will also reduce the time others spend understanding the tests.
Separate each testing point of the functions into separate tests.
Additionally, if its necessary for permit to be mixed into zapOut explain why. It should work without permit as well, so that should be tested too.
| if (_token != ETH) { | ||
| _permit(_token, _permitParams); | ||
| IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount); | ||
| _approveTokenIfNeeded(_token, swapper.tokenTransferProxy(), _amount); |
There was a problem hiding this comment.
I understand your point. I prefer to avoid doing this. Have added tags for Dhruvin + Mario to weigh in.
| address(this), | ||
| _amount, | ||
| _zapParams.accountsProof, | ||
| _zapParams.codesProof |
There was a problem hiding this comment.
This should be set in, and read from, storage.
The codesProof is for the OptyFiZapper so this contract can hold it.
It needs a permissioned setter function, and a getter.
spec/OptyFiZapper.behaviour.ts
Outdated
| expect(await vault.totalSupply()).to.eq(_previousVaultBalance.add(expectedReturns[1])); | ||
| expect(await vault.totalDeposits(maker.address)).to.eq(_previousBalance.add(expectedReturns[1])); | ||
| // the vault shares VT will be same as total supply is zero | ||
| expect(await vault.balanceOf(maker.address)).to.eq(_previousBalance.add(expectedReturns[1])); |
There was a problem hiding this comment.
Are you testing locally on HH or using a forked mainnet?
If the former, please change so you use the latter - this will simulate the real life situation more accurately.
spec/OptyFiZapper.behaviour.ts
Outdated
| ); | ||
| // await vaultToken.connect(maker).approve(instance.address, usdcZapOutAmount); | ||
| //zap UNI -> opUSDCgrow | ||
| await expect( |
There was a problem hiding this comment.
Are there no relevant events emitted?
Co-authored-by: NouDaimon <81045441+NouDaimon@users.noreply.github.com>
|
3 participants
PR Review:
Zapper contracts
Zapper tests