Setting up HH framework for Litmus Core development #1

dimpar · 2023-10-19T15:50:04Z

Added new hh toolbox
Added deployemnt plugin
Added helper scripts
Added linters
Added placeholder dirs for contracts and tests

nkuba

🔥:fire::fire:
The first round of comments.

nkuba · 2023-10-24T10:52:52Z

core/test/Litmus.ts

How about naming the file Litmus.test.ts to establish the ContractName.test.ts convention?

nkuba · 2023-10-24T10:56:10Z

core/scripts/deploy.ts

Let's rename the directory to deploy and rename the file to 01_deploy_litmus.ts.

nkuba · 2023-10-24T11:09:43Z

core/.eslintrc

+{
+  "root": true,
+  "extends": ["@thesis-co"],
+  "parserOptions": {
+    "ecmaVersion": 2017,
+    "sourceType": "module"
+  },
+  "env": {
+    "es6": true,
+    "mocha": true
+  },
+  "rules": {
+    "new-cap": "off",
+    "import/no-extraneous-dependencies": "off",
+    "@typescript-eslint/no-use-before-define": "off",
+    "no-plusplus": ["error", { "allowForLoopAfterthoughts": true }]
+  }
+}


How about we keep the configuration here minimal and see what we can pull from https://github.com/keep-network/keep-core/blob/main/solidity/ecdsa/.eslintrc to the common @thesis-co rules?

Suggested change

{

"root": true,

"extends": ["@thesis-co"],

"parserOptions": {

"ecmaVersion": 2017,

"sourceType": "module"

},

"env": {

"es6": true,

"mocha": true

},

"rules": {

"new-cap": "off",

"import/no-extraneous-dependencies": "off",

"@typescript-eslint/no-use-before-define": "off",

"no-plusplus": ["error", { "allowForLoopAfterthoughts": true }]

}

}

{

"root": true,

"extends": ["@thesis-co"]

}

Sure, but e.g. this rule "import/no-extraneous-dependencies": "off" has to stay, otherwise we get errors that some of the dependencies in package.json are in devDependencies instead of being in dependencies.

Sure, makes perfect sense! Could we disable it here only for specific files?

"import/no-extraneous-dependencies": [ "error", { devDependencies: ["hardhat.config.ts", "test/**"] }, ],

core/.eslintignore

core/.gitignore

core/hardhat.config.ts

core/export.json

core/package.json

nkuba · 2023-10-25T08:37:10Z

core/package.json

+    "@nomicfoundation/hardhat-verify": "^1.0.0",
+    "@nomiclabs/hardhat-etherscan": "^3.1.7",
+    "@nomiclabs/hardhat-waffle": "^2.0.6",
+    "@openzeppelin/hardhat-upgrades": "^2.3.3",


Let's remove this dependency until we really need it.

Suggested change

"@openzeppelin/hardhat-upgrades": "^2.3.3",

@dimpar can we remove this dependency?

core/package.json

core/slither.config.json

core/.gitignore

nkuba · 2023-10-25T09:06:57Z

core/package.json

+    "lint:eslint": "eslint .",
+    "lint:fix:eslint": "eslint . --fix",
+    "lint:sol": "solhint 'contracts/**/*.sol'",
+    "lint:fix:sol": "solhint 'contracts/**/*.sol' --fix",


I lean towards adding :fix as a suffix for the scripts, as it seams cleaner on the list.

Suggested change

"lint:eslint": "eslint .",

"lint:fix:eslint": "eslint . --fix",

"lint:sol": "solhint 'contracts/**/*.sol'",

"lint:fix:sol": "solhint 'contracts/**/*.sol' --fix",

"lint:eslint": "eslint .",

"lint:eslint:fix": "eslint . --fix",

"lint:sol": "solhint 'contracts/**/*.sol'",

"lint:sol:fix": "solhint 'contracts/**/*.sol' --fix",

Sure, and I would simplify this structure a bit as well. See e.g.

"lint:eslint": "eslint .",

the key is longer than the value itself. I know there's a pattern, but having command just like yarn eslint is a very straightforward too IMO.

I changed to the following:

"format": "npm run eslint && npm run solhint && npm run prettier:config", "format:fix": "npm run eslint:fix && npm run solhint:fix && npm run prettier:config:fix", "eslint": "eslint .", "eslint:fix": "eslint . --fix", "solhint": "solhint 'contracts/**/*.sol' && prettier --check 'contracts/**/*.sol'", "solhint:fix": "solhint 'contracts/**/*.sol' --fix && prettier --write 'contracts/**/*.sol'", "prettier:config": "prettier --check '**/*.@(json)'", "prettier:config:fix": "prettier --write '**/*.@(json)'",

Shouldn't we hide the tool name in the script name? IMO we should be generic. If we changed the linting tool, we wouldn't have to change script name in package.json in the future. I know it's a rather unlikely case but who knows 😆.

@r-czajkowski made a good point.

Ok, agree, making it generic makes sense, although once they are set I don't think we'll ever change them. They served us well in other projects.
Anyway, lint:fix:eslint points to a concrete tool. lint:js like in the comment here implies that it's for JavaScript. But we might as well use it for TypeScript. I'd propose simply leaving it as lint and lint:fix for all the JS/TS/JSX files, and add extensions like lint:sol or lint:config in all other cases. cc73eca

With lint:js I think about the group of all file types from the JavaScript family (including *.js, *.ts, *.jsx, *.cjs), not about a specific extension.

I think it would be consistent to define lint:js, as we have lint:sol for all Solidity files, and lint:config for JSON and YAML files.

Right, but lint:js implies JavaScript. Ok, if you feel strongly about lint:js I can change it. Consistency also comes with just having lint at the front.

I lean towards lint:js, would it be acceptable to you? 🙏🏻

core/.prettierrc.js

core/package.json

dimpar · 2023-10-26T12:27:27Z

@nkuba I agreed with most of your comments. There are a couple of places where I changed code a bit differently than your suggestions, pls see my responses. There are a lot of small changes and I decided to put everything in one commit since this is a config setup PR. See 6cc518c

r-czajkowski

Left some comments to take a look at before the merge.

core/.gitignore

core/.eslintignore

core/.prettierignore

core/export.json

nkuba · 2023-10-26T18:30:13Z

core/.eslintrc

+  "root": true,
+  "extends": ["@thesis-co"],
+  "rules": {
+    "import/no-extraneous-dependencies": "off"


Suggested change

"import/no-extraneous-dependencies": "off"

"import/no-extraneous-dependencies": [

"error",

{ devDependencies: ["hardhat.config.ts", "test/**"] },

],

Why would you want to limit it to ["hardhat.config.ts", "test/**"] only? This rule says that if e.g. "@nomicfoundation/hardhat-toolbox" is imported in other files than specified above then throw an error. We are not going to put "@nomicfoundation/hardhat-toolbox" under dependencies in package.json just because of some other file imports it. Am I missing something?

If we set the import/no-extraneous-dependencies to off, we disable the check completely. But we want to have it.
With the configuration I proposed, the rule will be modified only for devDependencies check in the files ["hardhat.config.ts", "test/**"]. All other files will be verified according to the default behavior.

Yea, I know, that's my point. Say we use import @nomicfoundation/hardhat-toolbox in a file other than ["hardhat.config.ts", "test/**"] and boom we have an error, because @nomicfoundation/hardhat-toolbox in under devDependencies and eslint wants it under dependencies. We will need to add a new file to this array again. Is your point having it enabled only for specific files because in case we add a new lib in a wrong "dependencies" key in package.json you want to see an error? If yes, I can buy that, but at the same time it leaves the problem described at the beginning. I guess we need to chose less evil. Having said that, you still prefer having this enabled only for certain files?

Is your point having it enabled only for specific files because in case we add a new lib in a wrong "dependencies" key in package.json you want to see an error?

Yes, exactly that. I don't want to have the dependencies we use in deployment script and tests to be pulled with dependencies with the package. dependencies should be only contracts dependencies, while all tests and scripts should be devDependencies.

core/.gitignore

nkuba · 2023-10-26T19:09:29Z

core/.solhint.json

@@ -0,0 +1,7 @@
+{
+  "extends": "keep",


I forked the solhint config under thesis organization: thesis/solhint-config#1.
We don't need to switch in this PR, but flagging it for the future.

nkuba · 2023-10-26T19:15:31Z

core/.solhint.json

+  "extends": "keep",
+  "plugins": [],
+  "rules": {
+    "func-visibility": ["error", { "ignoreConstructors": true }]


We won't need to define this rule after thesis/solhint-config#2 is merged.

core/contracts/Litmus.sol

core/package.json

- Added new hh toolbox - Added deployemnt plugin - Added helper scripts - Added linters - Added placeholder dirs for contracts and tests

There are a lot of minor changes to include each in seperate commits. This is a part of the overall project structure. Changes include: - established pattern for the deployment scripts and testing files - cleanup in the package.json and hardhat.config.js - cleanup in prettier, eslint, slither and mocha config files - cleanup in gitignore

In this PR we add: - New HardHat toolbox - Deployment plugin - Helper scripts - Linters for JavaScript-like files, Solidity and JSON - Placeholder directories for contracts and tests This PR cherry picked all of the commits from #1 because of some rebasing issues that auto-closed that PR. I've tried a couple of resurrecting commands but they didn't help unfortunately. Most of the code was reviewed in that closed PR. The last reviewed commit was `3c4336e` and 2 commits were added since then addressing some of the review comments.

nkuba · 2023-10-31T10:56:44Z

Continued in #7.

Initial commit

2783274

dimpar requested review from nkuba and r-czajkowski October 19, 2023 15:50

nkuba reviewed Oct 24, 2023

View reviewed changes

nkuba reviewed Oct 25, 2023

View reviewed changes

core/.gitignore Outdated Show resolved Hide resolved

nkuba reviewed Oct 25, 2023

View reviewed changes

core/.prettierrc.js Outdated Show resolved Hide resolved

nkuba reviewed Oct 26, 2023

View reviewed changes

core/package.json Outdated Show resolved Hide resolved

nkuba mentioned this pull request Oct 26, 2023

Add pre-commit hooks configuration #3

Closed

r-czajkowski mentioned this pull request Oct 26, 2023

Token Staking #4

Closed

r-czajkowski requested changes Oct 26, 2023

View reviewed changes

core/.gitignore Outdated Show resolved Hide resolved

core/.eslintignore Outdated Show resolved Hide resolved

core/.prettierignore Outdated Show resolved Hide resolved

core/export.json Outdated Show resolved Hide resolved

nkuba mentioned this pull request Oct 26, 2023

Define GitHub Actions workflow for Core module #6

Closed

nkuba reviewed Oct 26, 2023

View reviewed changes

core/package.json Outdated Show resolved Hide resolved

dimpar added 7 commits October 30, 2023 16:54

Setting up HH framework for Litmus core development

607f53b

- Added new hh toolbox - Added deployemnt plugin - Added helper scripts - Added linters - Added placeholder dirs for contracts and tests

Making scripting name generic

d355b44

Removing export.json from repository

48d3158

Adding empty line at the end and sorting alphabetically

15610dd

Changing the license in a placeholder contract

1661bde

Pinning Solidity version to 0.8.21

eb4c87e

dimpar closed this Oct 30, 2023

dimpar force-pushed the core-init branch from 81a4132 to eb4c87e Compare October 30, 2023 15:55

dimpar mentioned this pull request Oct 30, 2023

Project setup #7

Merged

pnicjanik mentioned this pull request Jul 10, 2024

disconnect mid-withdrawal presents multiple signature retries with different id #592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting up HH framework for Litmus Core development #1

Setting up HH framework for Litmus Core development #1

dimpar commented Oct 19, 2023

nkuba left a comment

nkuba Oct 24, 2023

nkuba Oct 24, 2023

nkuba Oct 24, 2023

dimpar Oct 26, 2023

nkuba Oct 26, 2023

nkuba Oct 25, 2023

nkuba Oct 30, 2023

nkuba Oct 25, 2023

dimpar Oct 26, 2023

r-czajkowski Oct 26, 2023

nkuba Oct 26, 2023

dimpar Oct 26, 2023

nkuba Oct 27, 2023

dimpar Oct 27, 2023

nkuba Oct 30, 2023

dimpar commented Oct 26, 2023

r-czajkowski left a comment

nkuba Oct 26, 2023

dimpar Oct 26, 2023

nkuba Oct 27, 2023

dimpar Oct 27, 2023

nkuba Oct 30, 2023

nkuba Oct 26, 2023

nkuba Oct 26, 2023

nkuba commented Oct 31, 2023

Setting up HH framework for Litmus Core development #1

Setting up HH framework for Litmus Core development #1

Conversation

dimpar commented Oct 19, 2023

nkuba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimpar commented Oct 26, 2023

r-czajkowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkuba commented Oct 31, 2023