Code cleanup #17
Code cleanup #17
Conversation
Will likely be picked up in audits as SWC-103 https://swcregistry.io/docs/SWC-103/
As per Solidity 0.8.0, ABI coder v2 is activated by default and as such it does not need to be specified in the code. Disregard this commit if wanting to be explicit in using ABI coder v2 instead of experimental ABIEncoderV2 or abicoder V1.
In `FeeCollector.sol`, `PositionFactory.sol` and `AdminService.sol`, both a fallback() and receive() function are specified. No code is executed in these contracts upon receiving ether, therefore, fallback() can be omitted as it is redundant.
| /** | ||
| * @notice Executes when native is sent to this contract through a non-existent function. | ||
| */ | ||
| fallback() external payable { } // solhint-disable-line no-empty-blocks |
There was a problem hiding this comment.
-
We can probably remove receive() everywhere too, right? I can't ever see a reason we want the contract to be able to accept ETH.
-
If so, we can remove the unit tests for both receive() and fallback() everywhere in the tests.
There was a problem hiding this comment.
-
There is no reason for the contract to receive ETH as I see it, so
receive()could also go. I think the only time ETH would be sent would be some kind of user error, sending to the wrong address or erroneously thinking they need to send ETH to interact with the protocol. -
Agreed. I can go through and remove those as well if that is what we want to do with removing both.
There was a problem hiding this comment.
For now, let's just remove the unit tests for fallback.
There was a problem hiding this comment.
fallback() removed, alongside the associated fuzz test for each.
Minor cleanup of code. Reasoning for each commit given in its comments.