Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(solhint-plugin-mud): create package #885

Merged
merged 4 commits into from
May 18, 2023
Merged

Conversation

dk1a
Copy link
Contributor

@dk1a dk1a commented May 17, 2023

fixes #711

seems to work well via cli (like via solhint in package.json of examples)

caveats:

  • does not work in vscode
  • has to be cjs-only, but it's fine because it should be imported only by solhint
  • solhint and its plugin examples are all in js, I may have made the first plugin with typescript. It is a bit incomplete (some any because solhint doesn't export any types) and relies on @solidity-parser/parser for some types (which solhint uses internally)

To make it work in https://github.com/juanfranblanco/vscode-solidity these 2 issues would need to be resolved (stale, but I could probably do this myself if we wanna prioritize this):
protofire/solhint#206
juanfranblanco/vscode-solidity#169

I've also tried hardhat extension but immediately got remapping problems before even getting to solhint plugins, but maybe worth exploring alternatives

Haven't looked into solium yet, that may also be worth exploring

@@ -40,7 +40,8 @@
"prettier-plugin-solidity": "^1.0.0-beta.19",
"rimraf": "^3.0.2",
"run-pty": "^4.0.3",
"solhint": "^3.3.7",
"solhint": "^3.4.1",
"solhint-plugin-mud": "file:../../../../packages/solhint-plugin-mud",
Copy link
Member

@holic holic May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should prob use link rather than file to match other mud deps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I was thinking I wanted a comment there, but afaik I can't, right?
This is intentional, how pnpm does links conflicts with how solhint requires plugins apparently (my guess is something about flat node_modules, took a while to narrow this down)

Copy link
Member

@holic holic May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we add this to templates, we need to adjust copy-templates.sh to also parse file: deps (not just link:)

although this probably wouldn't be captured there anyway since it doesn't have a @latticexyz/ prefix

@dk1a dk1a marked this pull request as ready for review May 17, 2023 22:23
@dk1a dk1a requested a review from alvrs as a code owner May 17, 2023 22:23
"rules": {
"compiler-version": ["error", ">=0.8.0"],
"avoid-low-level-calls": "off",
"no-inline-assembly": "off",
"func-visibility": ["warn", { "ignoreConstructors": true }],
"no-empty-blocks": "off",
"no-complex-fallback": "off"
"no-complex-fallback": "off",
"mud/no-msg-sender": "error"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any way to make this the default if you enable/use the plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a mud extension with defaults, that would be another package in a followup PR

@holic
Copy link
Member

holic commented May 17, 2023

these 2 issues would need to be resolved (stale, but I could probably do this myself if we wanna prioritize this)

Probably worth figuring this out? cc @alvrs @ludns

@juanfranblanco was quite responsive with the foundry monorepo support once I got his attention :)

@dk1a
Copy link
Contributor Author

dk1a commented May 17, 2023

regarding the issues - from what I understood the root one is on solhint side, they'd need to significantly change how plugins are loaded
vscode extension would come after

@juanfranblanco
Copy link

The issue with plugins was due to not being able to run them separately (IE providing a path) the package runs in the context of the extension not on the users workspace so it cannot find them. If this is changed it could be configured to look for plugins in a specific folder i.e user's node modules in her project.

alvrs
alvrs previously approved these changes May 18, 2023
Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! VSCode support would be amazing but not blocking for this PR

packages/solhint-plugin-mud/package.json Outdated Show resolved Hide resolved
Co-authored-by: alvarius <89248902+alvrs@users.noreply.github.com>
@alvrs alvrs merged commit 9149057 into main May 18, 2023
@alvrs alvrs deleted the dk1a/solhint-plugin-mud branch May 18, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CLI] Linter to prevent use of msg.sender in Systems (and other antipatterns)
4 participants