Add custom linting rules #4132

lhemerly · 2023-03-22T23:23:40Z

Adds a custom plugin for Solhint that implements the project's Solidity guidelines.

frangio · 2023-03-22T23:47:27Z

Nice @lhemerly! This is appreciated.

Is it possible to have the "plugin" as a JS file in this repository, rather than a separate npm package? I would like to do that as it will be easier for us to maintain and evolve.

lhemerly · 2023-03-23T00:28:14Z

As far as I know it is not possible right now as you can't set-up the plugin path in solhint:
Solhint - Issue#206

It is hardcoded to look up for solhint-plugin-${pluginName} in the node_modules folder:
Solhint loadPlugin Function

I may try a PR on Solhint for custom plugins paths later

Fixes OpenZeppelin#3919 Add solhint-plugin-ozcontracts to devDependencies Add ozonctracts to .solhint.json plugins Add oz-contracts-custom to .solhint.json rules #### PR Checklist - [ x] Tests - [ ] Documentation - [ ] Changeset entry (run `npx changeset add`)

lhemerly · 2023-03-24T02:27:23Z

Just a follow up, PR in solhint created:
Solhint PR for Issue #206

frangio · 2023-03-24T22:06:40Z

Maybe we could make it work by defining the package locally in the repo? Not exactly but sort of monorepo style.

lhemerly · 2023-03-25T00:45:51Z

The way I can imagine making it work as solhint is right now is to take the "solhint-plugin-ozcontracts" folder inside node_modules out of the gitignore and work with the .js file there as part of the repo. The way npm (or yarn) install works it won't change that folder as long as we remove and keep it out of package.json.

If you think it is ok to go with that I can make the necessary changes

frangio · 2023-06-16T17:55:40Z

Putting in node_modules is not ideal. We should be able to put it in a subdirectory somewhere in the repo and add "solhint-plugin-custom": "file:./solhint-plugin" in the dependencies.

…-contracts into lhemerly/issue3919

frangio · 2023-07-06T03:17:47Z

@lhemerly I've vendored your plugin into this repository like I suggested.

This should be a great start to begin to build more custom linting rules. Thanks!

frangio · 2023-07-06T03:20:18Z

@Amxx @ernestognw Note that this will allow us to have internal constants without leading underscore moving forward.

frangio · 2023-07-06T03:21:58Z

scripts/solhint-custom/index.js

+    }
+  },
+
+  // TODO: re-enable and fix


We have some code that causes this rule to fail so I'm proposing to merge and enable it later.

solhint.config.js

Co-authored-by: Hadrien Croubois <[email protected]>

Amxx · 2023-07-06T15:47:42Z

scripts/solhint-custom/index.js

+        if (!/^_/.test(node.name)) {
+          this.error(node, 'Private variables must be prefixed with underscore');
+        }
+      }


Should we also have some rule that if the stateVar is constantOrImmutable, then it must NOT start with an underscore ? or do we accept both ?

Suggested change

}

} else if (node.isStateVar && constantOrImmutable) {

if (/^_/.test(node.name)) {

this.error(node, 'Constant and immutable variables should not be prefixed with underscore');

}

}

I agree, but this causes the rule to fail in a few places.

I added the rule but commented it out, and I'd propose to do another PR next week fixing those things. Does this sound ok?

So we will have 2 follow up PRs that :

enable the rule

fix the code
?

That sounds ok

Yeah or just one PR doing the two.

Maybe the rule should also check that the name is capitalized.

That should be already handled by the casing rules in solhint.config.js, specifically const-name-snakecase.

lhemerly · 2023-07-06T16:10:15Z

Just an update, development branch in solhint adjusted plugins to be in any path:
protofire/solhint#424 (comment)

ernestognw · 2023-07-10T15:45:09Z

solhint.config.js

+
+module.exports = {
+  plugins: ['openzeppelin'],
+  rules: Object.fromEntries(rules.map(r => [r, 'error'])),


Why not define it like an object?

const rules = { 'no-unused-vars': 'error', ... }

Not a blocker because everything shows an "error", I just wanted to point out that the array doesn't seem to be needed

We need to use fromEntries in some way because we get the names of the custom rules dynamically:

customRules.map(r => `openzeppelin/${r.ruleId}`)

The rest could be an object, but I had a feeling that this was clearer? Let me know if it's too confusing.

gitpoap-bot · 2023-07-10T20:26:07Z

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 OpenZeppelin Contracts Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

This comment was marked as off-topic.

Sign in to view

frangio added the ignore-changeset label Mar 22, 2023

Add custom linting rules

6c08f86

Fixes OpenZeppelin#3919 Add solhint-plugin-ozcontracts to devDependencies Add ozonctracts to .solhint.json plugins Add oz-contracts-custom to .solhint.json rules #### PR Checklist - [ x] Tests - [ ] Documentation - [ ] Changeset entry (run `npx changeset add`)

frangio mentioned this pull request Jun 15, 2023

Do not require internal constant to start with an underscore #4355

Open

frangio added 4 commits July 5, 2023 22:45

vendor plugin code

7e1fe6d

refactor and improve rules

b3e6d79

Merge branch 'master' of https://github.com/OpenZeppelin/openzeppelin…

61194ec

…-contracts into lhemerly/issue3919

lint

9c7dc19

Merge branch 'master' into lhemerly/issue3919

cf8a1b1

frangio requested review from Amxx and ernestognw July 6, 2023 03:18

lint

8338d2a

frangio reviewed Jul 6, 2023

View reviewed changes

Amxx reviewed Jul 6, 2023

View reviewed changes

solhint.config.js Outdated Show resolved Hide resolved

Amxx reviewed Jul 6, 2023

View reviewed changes

solhint.config.js Outdated Show resolved Hide resolved

Update solhint.config.js

2e54ac2

Co-authored-by: Hadrien Croubois <[email protected]>

Amxx reviewed Jul 6, 2023

View reviewed changes

adjust leading underscore rule

276dbae

ernestognw approved these changes Jul 10, 2023

View reviewed changes

frangio merged commit cd981f6 into OpenZeppelin:master Jul 10, 2023

0xVolosnikov mentioned this pull request Aug 26, 2023

Use leading underscore solhint rule for private constants #4542

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom linting rules #4132

Add custom linting rules #4132

lhemerly commented Mar 22, 2023 •

edited by frangio

Loading

This comment was marked as off-topic.

frangio commented Mar 22, 2023

lhemerly commented Mar 23, 2023 •

edited

Loading

lhemerly commented Mar 24, 2023

frangio commented Mar 24, 2023

lhemerly commented Mar 25, 2023

frangio commented Jun 16, 2023

frangio commented Jul 6, 2023

frangio commented Jul 6, 2023

frangio Jul 6, 2023

Amxx Jul 6, 2023 •

edited

Loading

frangio Jul 6, 2023

Amxx Jul 6, 2023

frangio Jul 6, 2023

Amxx Jul 6, 2023

frangio Jul 6, 2023

lhemerly commented Jul 6, 2023

ernestognw Jul 10, 2023

frangio Jul 10, 2023 •

edited

Loading

gitpoap-bot bot commented Jul 10, 2023

-      }
+      } else if (node.isStateVar && constantOrImmutable) {
+        if (/^_/.test(node.name)) {
+          this.error(node, 'Constant and immutable variables should not be prefixed with underscore');
+        }
+      }

Add custom linting rules #4132

Add custom linting rules #4132

Conversation

lhemerly commented Mar 22, 2023 • edited by frangio Loading

This comment was marked as off-topic.

frangio commented Mar 22, 2023

lhemerly commented Mar 23, 2023 • edited Loading

lhemerly commented Mar 24, 2023

frangio commented Mar 24, 2023

lhemerly commented Mar 25, 2023

frangio commented Jun 16, 2023

frangio commented Jul 6, 2023

frangio commented Jul 6, 2023

frangio Jul 6, 2023

Choose a reason for hiding this comment

Amxx Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

frangio Jul 6, 2023

Choose a reason for hiding this comment

Amxx Jul 6, 2023

Choose a reason for hiding this comment

frangio Jul 6, 2023

Choose a reason for hiding this comment

Amxx Jul 6, 2023

Choose a reason for hiding this comment

frangio Jul 6, 2023

Choose a reason for hiding this comment

lhemerly commented Jul 6, 2023

ernestognw Jul 10, 2023

Choose a reason for hiding this comment

frangio Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

gitpoap-bot bot commented Jul 10, 2023

lhemerly commented Mar 22, 2023 •

edited by frangio

Loading

lhemerly commented Mar 23, 2023 •

edited

Loading

Amxx Jul 6, 2023 •

edited

Loading

frangio Jul 10, 2023 •

edited

Loading