makerdao / spells-mainnet

Staging repo for MakerDAO weekly executive spells
GNU Affero General Public License v3.0
122 stars 50 forks source link

Chore: fix compiler warnings #440

Closed amusingaxl closed 2 days ago

amusingaxl commented 2 days ago

Currently we get the following compiler warnings when running the spell tests:

./scripts/test-dssspell-forge.sh no-match="" match="testDaoResolutions" block=""
Using DssExecLib at: 0x8De6DDbCd5053d32292AAA0D2105A32d108484a6
[⠊] Compiling...
[⠃] Compiling 3 files with Solc 0.8.16
[⠊] Solc 0.8.16 finished in 2.77s
Compiler run successful with warnings:
Warning (2018): Function state mutability can be restricted to view
   --> src/DssSpell.t.base.sol:983:5:
    |
983 |     function _getOSMPrice(address pip) internal returns (uint256) {
    |     ^ (Relevant source part starts here and spans across multiple lines).

Warning (2018): Function state mutability can be restricted to view
   --> src/DssSpell.t.base.sol:999:5:
    |
999 |     function _getUNIV2LPPrice(address pip) internal returns (uint256) {
    |     ^ (Relevant source part starts here and spans across multiple lines).

Warning (2018): Function state mutability can be restricted to pure
    --> src/DssSpell.t.base.sol:2174:5:
     |
2174 |     function _getSignatures(bytes32 signHash) internal returns (bytes memory signatures, address[] memory signers) {
     |     ^ (Relevant source part starts here and spans across multiple lines).

Warning (2018): Function state mutability can be restricted to view
    --> src/DssSpell.t.base.sol:2366:5:
     |
2366 |     function _checkTransferrableVestMkrAllowance() internal {
     |     ^ (Relevant source part starts here and spans across multiple lines).

Warning (2018): Function state mutability can be restricted to view
    --> src/DssSpell.t.base.sol:2722:5:
     |
2722 |     function _testContractSize() internal {
     |     ^ (Relevant source part starts here and spans across multiple lines).

Warning (2018): Function state mutability can be restricted to view
    --> src/DssSpell.t.sol:1036:5:
     |
1036 |     function testDaoResolutions() public { // add the `skipped` modifier to skip
     |     ^ (Relevant source part starts here and spans across multiple lines).

Notice that testDaoResolutions can be restricted to view if it doesn't have the skipped modifier to it, as vm.skip() is not a view function. In this case, we could simply ignore the warning or update the instruction to replace the view modifier with skipped when necessary.

I also took the opportunity to make some small improvements on the base tests.