feat: everything done #1
Conversation
There was a problem hiding this comment.
GREAT JOB!! You went ahead and completed the challenge and all of the bonuses, well done.
Regarding the tests, I believe you could have done them a bit tidier, maybe by separating e2e from unit tests. There is a library that we developed in-house that may help your with simplifying your tests: https://github.com/defi-wonderland/smock
I left you some comments all over, most of them are not a must, they are just for your personal improvement.
Again, great great job 👏
| hardhat: { | ||
| forking: { | ||
| url: urlAlchemy, | ||
| block: 13006453 |
There was a problem hiding this comment.
What if 2 tests would require to fork a different block number? I would prefer to do enable: false here and to reset it independently in each test
| @@ -0,0 +1,19 @@ | |||
| /** | |||
| * @type import('hardhat/config').HardhatUserConfig | |||
There was a problem hiding this comment.
I see you like types, why not using typescript then?
|
|
||
| require("@nomiclabs/hardhat-waffle"); | ||
| // require("hardhat-gas-reporter"); | ||
| let { urlAlchemy } = require("./secrets.json"); |
There was a problem hiding this comment.
usually, you want to create a secrets.example.json that is not gitignored, so the people can know what is the structure of the file without actually looking at the code
| */ | ||
|
|
||
| require("@nomiclabs/hardhat-waffle"); | ||
| // require("hardhat-gas-reporter"); |
There was a problem hiding this comment.
Why not using the reporter finally?
| @@ -0,0 +1,10 @@ | |||
| //SPDX-License-Identifier: Unlicense | |||
There was a problem hiding this comment.
It's a good practice to keep the name of the file the same as the name of the contract, in this case GenericERC20.sol
|
|
||
| if (balanceFromToken > 0) { | ||
| fromTokenBalances.set(key, 0); | ||
| uint256 balanceToToken = fromTokenBalances.get(key); |
There was a problem hiding this comment.
How does this work? You just set the balance to 0 in the previous line...
Maybe you meant toTokenBalances
| whaleSigner2 = await ethers.getSigner(whaleAddress2); | ||
|
|
||
| //Deploy library | ||
| this.IterableMapping = await ethers.getContractFactory("IterableMapping"); |
There was a problem hiding this comment.
Is it worth the headache using the IterableMapping in this case? Just asking...
| await this.swapper.connect(whaleSigner).swap(); | ||
| let balanceAfterSwapFromToken = ethers.utils.formatUnits(await this.swapper.connect(whaleSigner).viewBalanceFromToken()); | ||
| let balanceAfterSwapToToken = ethers.utils.formatUnits(await this.swapper.connect(whaleSigner).viewBalanceToToken()); | ||
| expect(Number(balanceAfterSwapFromToken)).equal(0); |
There was a problem hiding this comment.
hardhat-waffle supports comparisson with big numbers, I don't think you need any Number(...)
|
|
||
| using IterableMapping for IterableMapping.Map; | ||
|
|
||
| address public DAI; |
There was a problem hiding this comment.
all variables should be camelCase, except for constants that should be all caps
| function work(uint256 deadline) public { | ||
| require(workable(), "You can't execute now"); | ||
| SwapperUNIV2.swap(deadline); | ||
| Keep3r.receipt(Keep3rAddress, msg.sender, 1 * 10 ** 18); |
There was a problem hiding this comment.
You can just do 1 ether instead of 1 * 10 ** 18
| return count; | ||
| } | ||
|
|
||
| function provide(uint256 amount, bool isNativeETH) external payable { |
There was a problem hiding this comment.
You could create two overloaded functions, for example:
function provide(uint256 amount) external
and
function provide() external payable
No description provided.