Contract checklist

[JasoonS]

Contracts:

• We must check that erc20 transfers return 'true', and think through the consequences of a transfer failing and if that will be detected. OpenZeppelin have this for example: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/SafeERC20.sol#L55 . Maybe it isn't necessary, if the users do something weird that causes a transfer to them to fail they just lose their money.
• When a function returns "true" or "false" depending on if it fails, we should wrap those function calls in a require(functionCall(), "The function called failed"). (we shouldn't necessarily do this everywhere, such as in the above point.

Tests:

• refactor before each (for basically all the tests) into a common reusable function.
• look for common actions that happen in the tests and write helper functions that make the tests easier to read (and refactor in the future). There will always be a balance to strike, so we must be careful not to over-engineer this refactoring. Tests could be bundled into these helper functions too. Some examples come to mind.
  • increasing iteration. await time.increase(time.duration.seconds(1801)); is more difficult to read than await increaseIterationBy(2); or maybe await increaseIterationTill(5);
  • deposit and join pool as user
  • deposit and submit project
  • increaseTimeAndDistrubuteFunds
    ... etc
• quite a few places let is used for variable declaration, where const should be used (since there is no intention to re-assign them).
• We can name accounts rather than using an index to make it more clear eg put const benefactorOne = accounts[2] at the top.
• There are quite a few Events we aren't testing.
• We should give a string for every single assert so when they fail we can quickly and easily see why. I will be very very strict on this in the future, it isn't always obvious what the test is trying to achieve, and putting that string there can really help.
• There are a few tests where no values are checked after a transaction and it is implicitly assumed that because the transaction doesn't revert it does the right thing. Something to be very aware of.
• We should strive to NEVER use hard coded values in the tests, eg 1801, or amounts.

Non essential:

• add a sol hint file for consistent sol formatting. (should be integrated into the pipeline too)
• add pipelines that test the contracts + check linting on each PR (also they should display the coverage report etc.)

General:

• Add test pipeline.
• Publish coverage artifact with github actions so it is easy to see in the PR. We can add a rule that a PR cannot be merged without 100% coverage etc etc. But more than that we can see the pull request.

[JasoonS]

@moose-code We should go through these some time. I guess almost all of them are fixed already