Skip to content

Commit

Permalink
chore: update linting configuration (#177)
Browse files Browse the repository at this point in the history
## Description

This PR revamps linting settings and adds linting to files in `script`
and `test`.
This led to the following changes:
- Updated `solhint` to the latest version. Most notably,
`no-unused-import` has been introduced.
- Ignored a few new rules coming from the update. This is mostly because
we don't want to touch `src/` yet, I expect these rules to be dropped
eventually.
- Created dedicated config files for linting tests and scrips.
(Unfortunately, I didn't find a way to make them depend on each other
without creating a dedicated NPM package for this purpose.)
- Enforce Solidity pragma `^0.8` in tests and scripts. There's no need
to be strict (mentioned in f64f5df).
- Updated `package.json` with the new linting commands; in particular,
they will be used in CI.
- Fixed a few linting issues.
- Removed `NonPayable.sol` (unused).

## Known issues

1. Linting relies on `package.json`, which will eventually be removed
from this repo.
2. The Solidity VSCode extension linting isn't using the same linter
version used in CI. For example, it signals `custom-errors` despite it
being disabled (but through `gas-custom-errors`)

## Things to keep in mind for the future

The following features (from [Solhint's issue
tracker](https://github.com/protofire/solhint/issues/)) would be
helpful:
- 359 (overrides in `.solhint.json`)
- 302 (extensible configs)
- 571 (enforce import ordering; this is being worked on!)
- 496 (`"no-empty-blocks": ["error", { "ignoreConstructors": true }]`)
- 588 (`no-duplicated-import`)

## Test Plan

CI.
  • Loading branch information
fedgiac authored Jul 22, 2024
1 parent 9958921 commit ae1dc49
Show file tree
Hide file tree
Showing 56 changed files with 273 additions and 65 deletions.
4 changes: 3 additions & 1 deletion .solhint.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
"plugins": ["prettier"],
"rules": {
"compiler-version": "off",
"no-global-import": "off",
"func-visibility": ["warn", { "ignoreConstructors": true }],
"gas-custom-errors": "off",
"immutable-vars-naming": "off",
"no-global-import": "off",
"prettier/prettier": "error"
}
}
8 changes: 5 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@
"build": "yarn build:sol && yarn build:ts",
"build:sol": "hardhat compile --force",
"build:ts": "tsc && tsc -p tsconfig.lib.esm.json && tsc -p tsconfig.lib.commonjs.json",
"lint": "yarn lint:sol && yarn lint:ts",
"lint:sol": "solhint 'src/contracts/**/*.sol'",
"lint": "yarn lint:sol:src && yarn lint:ts && yarn lint:sol:test && yarn lint:sol:script",
"lint:sol:script": "solhint --max-warnings 0 --config ./script/.solhint.json 'script/**/*.sol'",
"lint:sol:src": "solhint --max-warnings 0 'src/**/*.sol'",
"lint:sol:test": "solhint --max-warnings 0 --config ./test/.solhint.json 'test/**/*.sol'",
"lint:ts": "eslint --max-warnings 0 .",
"test": "hardhat test",
"test:ignored-in-coverage": "MOCHA_CONF='ignored in coverage' hardhat test",
Expand Down Expand Up @@ -77,7 +79,7 @@
"prettier": "^2.8.7",
"prettier-plugin-solidity": "^1.1.3",
"sinon": "^15.0.3",
"solhint": "^3.4.1",
"solhint": "^5.0.0",
"solhint-plugin-prettier": "^0.0.5",
"solidity-coverage": "^0.8.2",
"ts-node": "^10.9.1",
Expand Down
11 changes: 11 additions & 0 deletions script/.solhint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"extends": "solhint:recommended",
"rules": {
"compiler-version": ["warn", "^0.8"],
"func-visibility": ["warn", { "ignoreConstructors": true }],
"gas-custom-errors": "off",
"immutable-vars-naming": "off",
"no-console": "off",
"no-global-import": "off"
}
}
2 changes: 1 addition & 1 deletion script/TransferOwnership.s.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {console} from "forge-std/Script.sol";

Expand Down
2 changes: 1 addition & 1 deletion script/interfaces/ERC173.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0
pragma solidity >=0.7.6 <0.9.0;
pragma solidity ^0.8;

// Copied from:
// <https://eips.ethereum.org/EIPS/eip-173#specification>
Expand Down
2 changes: 1 addition & 1 deletion script/lib/NetworksJson.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {Script} from "forge-std/Script.sol";

Expand Down
16 changes: 16 additions & 0 deletions test/.solhint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"extends": "solhint:recommended",
"rules": {
"compiler-version": ["warn", "^0.8"],
"const-name-snakecase": "off",
"func-name-mixedcase": "off",
"func-visibility": ["warn", { "ignoreConstructors": true }],
"gas-custom-errors": "off",
"immutable-vars-naming": "off",
"no-global-import": "off",
"no-inline-assembly": "off",
"one-contract-per-file": "off",
"reason-string": "off",
"state-visibility": "off"
}
}
2 changes: 1 addition & 1 deletion test/GPv2AllowListAuthenticator/AddSolver.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {GPv2AllowListAuthentication} from "src/contracts/GPv2AllowListAuthentication.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2AllowListAuthenticator/Helper.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2AllowListAuthenticator/InitializeManager.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {GPv2AllowListAuthentication} from "src/contracts/GPv2AllowListAuthentication.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2AllowListAuthenticator/IsSolver.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {Helper} from "./Helper.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2AllowListAuthenticator/RemoveSolver.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {GPv2AllowListAuthentication} from "src/contracts/GPv2AllowListAuthentication.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2AllowListAuthenticator/SetManager.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {GPv2AllowListAuthentication} from "src/contracts/GPv2AllowListAuthentication.sol";

Expand Down
4 changes: 3 additions & 1 deletion test/GPv2Interaction/Execute.t.sol
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {Helper} from "./Helper.sol";

import {GPv2Interaction} from "src/contracts/libraries/GPv2Interaction.sol";

// Intentionally empty contract.
// solhint-disable-next-line no-empty-blocks
contract NonPayable {}

contract Transfer is Helper {
Expand Down
3 changes: 1 addition & 2 deletions test/GPv2Interaction/Helper.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity >=0.7.6 <0.9.0;
pragma abicoder v2;
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2Interaction/Selector.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {Helper} from "./Helper.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2SafeERC20/Helper.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2SafeERC20/Transfer.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {IERC20} from "src/contracts/interfaces/IERC20.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2SafeERC20/TransferFrom.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {IERC20} from "src/contracts/interfaces/IERC20.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2Signing/Helper.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {IERC20} from "src/contracts/interfaces/IERC20.sol";
import {GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol";
Expand Down
4 changes: 1 addition & 3 deletions test/GPv2VaultRelayer/BatchSwapWithFee.FeeTransfer.t.sol
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;

import {
GPv2VaultRelayer, GPv2Transfer, IERC20, IVault, GPv2Transfer, GPv2Order
} from "src/contracts/GPv2VaultRelayer.sol";
import {GPv2Transfer, IERC20, IVault, GPv2Transfer, GPv2Order} from "src/contracts/GPv2VaultRelayer.sol";

import {BatchSwapWithFeeHelper} from "./Helper.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2VaultRelayer/Helper.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/GPv2VaultRelayer/TransferFromAccounts.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {IERC20, IVault, GPv2Transfer, GPv2Order} from "src/contracts/GPv2VaultRelayer.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/libraries/Bytecode.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {Vm} from "forge-std/Test.sol";
import {Bytes} from "./Bytes.sol";
Expand Down
2 changes: 1 addition & 1 deletion test/libraries/Bytes.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

library Bytes {
function slice(bytes memory d, uint256 offset, uint256 length) internal pure returns (bytes memory) {
Expand Down
2 changes: 1 addition & 1 deletion test/libraries/Order.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {IERC20, GPv2Order} from "src/contracts/libraries/GPv2Order.sol";
import {GPv2Trade} from "src/contracts/libraries/GPv2Trade.sol";
Expand Down
2 changes: 1 addition & 1 deletion test/libraries/Settlement.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {GPv2Settlement} from "src/contracts/GPv2Settlement.sol";
import {SettlementEncoder} from "./encoders/SettlementEncoder.sol";
Expand Down
2 changes: 1 addition & 1 deletion test/libraries/Sign.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {Vm} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/libraries/Trade.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {IERC20, GPv2Order, GPv2Trade, GPv2Signing} from "src/contracts/mixins/GPv2Signing.sol";
import {Sign} from "./Sign.sol";
Expand Down
3 changes: 2 additions & 1 deletion test/libraries/encoders/SettlementEncoder.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {Vm} from "forge-std/Test.sol";

Expand Down Expand Up @@ -106,6 +106,7 @@ library SettlementEncoder {

// All the order refunds are encoded in the POST interactions so we take some liberty and
// use a short variable to represent the POST stage.
// solhint-disable-next-line var-name-mixedcase
uint256 POST = uint256(InteractionStage.POST);
GPv2Interaction.Data[] memory postInteractions =
new GPv2Interaction.Data[](state.interactions[POST].length + r.length);
Expand Down
2 changes: 1 addition & 1 deletion test/libraries/encoders/SwapEncoder.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {Vm} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/libraries/encoders/TokenRegistry.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {IERC20, GPv2Order} from "src/contracts/libraries/GPv2Order.sol";

Expand Down
3 changes: 1 addition & 2 deletions test/reader/AllowListStorageReader.t.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later

pragma solidity ^0.8.24;
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/reader/SettlementStorageReader.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/reader/StorageAccessible.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {Test, Vm} from "forge-std/Test.sol";
import {StorageAccessibleWrapper, ExternalStorageReader} from "./StorageAccessibleWrapper.sol";
Expand Down
3 changes: 2 additions & 1 deletion test/reader/StorageAccessibleWrapper.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import "src/contracts/mixins/StorageAccessible.sol";

Expand All @@ -23,6 +23,7 @@ contract StorageAccessibleWrapper is StorageAccessible {
mapping(uint256 => uint256) qux;
FooBar foobar;

// solhint-disable-next-line no-empty-blocks
constructor() {}

function setFoo(uint256 foo_) public {
Expand Down
2 changes: 1 addition & 1 deletion test/reader/StorageReadable.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/reader/ViewStorageAccessible.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.0;
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/script/TransferOwnership.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

Expand Down
2 changes: 1 addition & 1 deletion test/script/lib/NetworksJson.t.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
pragma solidity ^0.8.26;
pragma solidity ^0.8;

import {Test} from "forge-std/Test.sol";

Expand Down
1 change: 1 addition & 0 deletions test/src/ERC20PresetPermit.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// solhint-disable-next-line compiler-version
pragma solidity ^0.7.6;

import "@openzeppelin/contracts/drafts/ERC20Permit.sol";
Expand Down
1 change: 1 addition & 0 deletions test/src/EventEmitter.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// solhint-disable-next-line compiler-version
pragma solidity >=0.7.6 <0.9.0;

contract EventEmitter {
Expand Down
1 change: 1 addition & 0 deletions test/src/FeeClaimingERC20.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: MIT
// solhint-disable-next-line compiler-version
pragma solidity ^0.7.6;

import "@openzeppelin/contracts/presets/ERC20PresetMinterPauser.sol";
Expand Down
1 change: 1 addition & 0 deletions test/src/GPv2AllowListAuthenticationV2.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// solhint-disable-next-line compiler-version
pragma solidity >=0.7.6 <0.9.0;

import "src/contracts/GPv2AllowListAuthentication.sol";
Expand Down
1 change: 1 addition & 0 deletions test/src/GPv2OrderTestInterface.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// solhint-disable-next-line compiler-version
pragma solidity >=0.7.6 <0.9.0;
pragma abicoder v2;

Expand Down
1 change: 1 addition & 0 deletions test/src/GPv2SettlementTestInterface.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// solhint-disable-next-line compiler-version
pragma solidity >=0.7.6 <0.9.0;
pragma abicoder v2;

Expand Down
1 change: 1 addition & 0 deletions test/src/GPv2SigningTestInterface.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// solhint-disable-next-line compiler-version
pragma solidity >=0.7.6 <0.9.0;
pragma abicoder v2;

Expand Down
1 change: 1 addition & 0 deletions test/src/GPv2TradeTestInterface.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// solhint-disable-next-line compiler-version
pragma solidity >=0.7.6 <0.9.0;
pragma abicoder v2;

Expand Down
1 change: 1 addition & 0 deletions test/src/GPv2TransferTestInterface.sol
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// solhint-disable-next-line compiler-version
pragma solidity >=0.7.6 <0.9.0;
pragma abicoder v2;

Expand Down
Loading

0 comments on commit ae1dc49

Please sign in to comment.