refactoring(cli && tests) - refactoring of the CLI package and integration tests

[ctrlc03]

fix #789 and fix #790

What has been done in this PR:

• rewrite cli code to be cleaner and compact
• removed argparse in favour of commander
• implemented interfaces for each of the command's arguments
• allowing commands to return values -> this way we can use them in the integration tests and we have a blueprint for a possible SDK or just use this in a frontend
• inline comments + typedoc across all of the code changes
• documented usage
• rewrote all of the tests to TS to improve readability as well as speed
• removed redundant test cases
• fixed wrong test cases (using stateIndex 0 while sending a message)
• refactored integration tests
• clean up of the integration tests
• use the new cli in the integration tests and do not read output from stdout/stderr
• removed several dependencies from these packages which are not needed
• do not deploy verifier contract twice (on maci deployment and poll deployment)
• ensure all recent changes are integrated
• allow integration tests to run without starting up hardhat first in the contracts folder

[yuetloo]

@ctrlc03 , have you tried creating a hardhat task that imports any maci-cli command? I'm getting error (see below). I believe the hardhat import in the getDefaultSigner and parseArtifact is the issue. Hadhat scripts work fine, just the hardhat task gives errors

Error HH9: Error while loading Hardhat's configuration.

You probably tried to import the "hardhat" module from your config or a file imported from it.
This is not possible, as Hardhat can't be initialized while its config is being defined.

To learn more about how to access the Hardhat Runtime Environment from different contexts go to https://hardhat.org/hre

For more info go to https://hardhat.org/HH9 or run Hardhat with --show-stack-traces```

[ctrlc03]

I have not, I will give it a try as soon as I get a chance

[baumstern]

I encountered a SyntaxError: Cannot use import statement outside a module when running node build/index.js using commit c4de587:

% npm install
% npm run bootstrap
% npm run build

% cd cli
% node build/index.js
/Users/daehyun/workspace/pse/maci/cli/hardhat.config.ts:1
import dotenv from "dotenv";
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at internalCompileFunction (node:internal/vm:73:18)
    at wrapSafe (node:internal/modules/cjs/loader:1178:20)
    at Module._compile (node:internal/modules/cjs/loader:1220:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:110:18)
    at importCsjOrEsModule (/Users/daehyun/workspace/pse/maci/contracts/node_modules/hardhat/internal/core/config/config-loading.js:24:26)
    at loadConfigAndTasks (/Users/daehyun/workspace/pse/maci/contracts/node_modules/hardhat/internal/core/config/config-loading.js:66:22)

Node.js v18.17.0

[baumstern]

Great work on this PR! The reduction in test runtime from an hour to 30 minutes is a significant improvement and streamlining the testing process by eliminating the need for a separate hardhat network is highly beneficial. I've also provided some feedback to further refine our project's overall code quality and maintainability.

[baumstern]

suggestion: here the CLI package references scripts directly from node_modules/maci-contracts/. Given that our codebase is a monorepo using Lerna, which sets up symlinks via lerna bootstrap, this setup could lead to issues. If the version specified in dependencies differs from the local maci-contracts version, Lerna might pull the package from npm instead of using the local symlink. This discrepancy could cause problems during development and testing, especially if there are version-specific dependencies or configurations. It's a potential area for tricky troubleshooting scenarios. We might want to consider revising this approach to ensure consistency and avoid such conflicts.

[ctrlc03]

What do you suggest here? I was thinking that users might install the cli package via npm and not have the full repo locally, so this way the package could work on its own

[baumstern]

suggestion: I noticed that our Mocha tests are throwing raw errors without using assertions. This results in unstructured and cluttered error logs, making it difficult to pinpoint the exact cause of test failures. For better clarity and maintainability, I suggest incorporating assertions (e.g., using assert or a similar library) in our test cases. This way, if a test fails, we get a clear, concise message about what was expected versus what was actually observed. It will make debugging more straightforward and improve the overall quality of our test suite:

    multiplePolls2
[✗] Could not find ./zkeys/ProcessMessages_10-2-1-2_test.wasm.
      9) should run the first poll
      ✔ should deploy two more polls (452ms)
      ✔ should publish messages to the second poll (736ms)
      ✔ should publish messages to the third poll (756ms)
[✗] Could not find ./zkeys/ProcessMessages_10-2-1-2_test.wasm.
      10) should complete the second poll
[✗] Could not find ./zkeys/ProcessMessages_10-2-1-2_test.wasm.
      11) should complete the third poll


  18 passing (1m)
  11 failing

  1) e2e tests
       test1
         should generate zk-SNARK proofs and verify them:
     
  Error: 
      at logError (ts/utils/theme.ts:46:11)
      at /Users/daehyun/workspace/pse/maci/cli/ts/commands/genProofs.ts:71:26
      at step (ts/commands/genProofs.ts:33:23)
      at Object.next (ts/commands/genProofs.ts:14:53)
      at /Users/daehyun/workspace/pse/maci/cli/ts/commands/genProofs.ts:8:71
      at new Promise (<anonymous>)
      at __awaiter (ts/commands/genProofs.ts:4:12)
      at genProofs (ts/commands/genProofs.ts:51:20)
      at Suite.<anonymous> (tests/e2e.test.ts:110:28)
      at step (tests/e2e.test.ts:33:23)
      at Object.next (tests/e2e.test.ts:14:53)
      at fulfilled (tests/e2e.test.ts:5:58)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      at runNextTicks (node:internal/process/task_queues:64:3)
      at listOnTimeout (node:internal/timers:538:9)
      at processTimers (node:internal/timers:512:7)

[ctrlc03]

[✗] Could not find ./zkeys/ProcessMessages_10-2-1-2_test.wasm. isn't that clear enough?

[ctrlc03]

I am throwing directly with the error text rather than an empty and confusing stack trace too (as suggested in a previous comment)

[baumstern]

Tests should explicitly assert the conditions they're checking for clarity and precision. If you intend to address this in a follow-up PR, please create an issue to track it.

[ctrlc03]

Yea tests should actually be properly reviewed in a separate piece of work imo (this PR goal was not that), there's loads we can improve and perhaps some of them are not so correct

[baumstern]

(+) I've observed that our current logging strategy frequently checks the quiet variable to decide whether to print log messages. This approach can be improved by adopting a more conventional logging level system. Using logging levels (like debug, info, warn, error) allows for more flexible and granular control over what gets logged.

There are several libraries that can help implement this, such as winston, bunyan, or pino. These packages provide robust logging capabilities with customizable levels, formats, and outputs, enhancing the overall logging strategy of our application.

[baumstern]

As I've gone through this PR, I've observed several areas that could benefit from future attention and refinement. While these don't necessarily need to be addressed within the scope of this current PR, they're important for the ongoing improvement of our project:

• Error Handling Techniques: From a CLI design perspective, we should consider how error messages are displayed to the end user. This includes refining error handling in the originating functions, how these errors are caught and processed, and ensuring that CLI commands present user-friendly error messages instead of raw output or stack traces.

Though not pressing for this PR, this point would be valuable to keep in mind for future development to elevate the quality and usability of our CLI.

[ctrlc03]

(+) I've observed that our current logging strategy frequently checks the quiet variable to decide whether to print log messages. This approach can be improved by adopting a more conventional logging level system. Using logging levels (like debug, info, warn, error) allows for more flexible and granular control over what gets logged.

There are several libraries that can help implement this, such as winston, bunyan, or pino. These packages provide robust logging capabilities with customizable levels, formats, and outputs, enhancing the overall logging strategy of our application.

I wanted to keep the production package a bit slimmer and avoid using third party libraries for that, what you think?

[ctrlc03]

I encountered a SyntaxError: Cannot use import statement outside a module when running node build/index.js using commit c4de587:

% npm install
% npm run bootstrap
% npm run build

% cd cli
% node build/index.js
/Users/daehyun/workspace/pse/maci/cli/hardhat.config.ts:1
import dotenv from "dotenv";
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at internalCompileFunction (node:internal/vm:73:18)
    at wrapSafe (node:internal/modules/cjs/loader:1178:20)
    at Module._compile (node:internal/modules/cjs/loader:1220:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:110:18)
    at importCsjOrEsModule (/Users/daehyun/workspace/pse/maci/contracts/node_modules/hardhat/internal/core/config/config-loading.js:24:26)
    at loadConfigAndTasks (/Users/daehyun/workspace/pse/maci/contracts/node_modules/hardhat/internal/core/config/config-loading.js:66:22)

Node.js v18.17.0

could you try to run it with ts-node instead please? this is due to hardhat.config.ts unfortunately. We need to think how this would work on a published npm package (how do users get access to the contracts ABI, how do they setup the config file locally, and how do they run the cli)

[baumstern]

(+) I've observed that our current logging strategy frequently checks the quiet variable to decide whether to print log messages. This approach can be improved by adopting a more conventional logging level system. Using logging levels (like debug, info, warn, error) allows for more flexible and granular control over what gets logged.
There are several libraries that can help implement this, such as winston, bunyan, or pino. These packages provide robust logging capabilities with customizable levels, formats, and outputs, enhancing the overall logging strategy of our application.

I wanted to keep the production package a bit slimmer and avoid using third party libraries for that, what you think?

It looks weird for me that we have to use if(!quiet) statement whenever we want manage log verbosity tbh. But let's keep it as it is for now since it gonna take our bandwidth to assess logging framework and implementing it. And at least we have a working custom logging system for now. If it turns out not working well later, then we could improve it from that point

[baumstern]

CI failed due to corrupted lock file in contract package:

> [email protected] compileSol
> ./scripts/compileSol.sh $1

Writing Merkle zeros contracts
Writing empty ballot tree root contract
Building contracts with Hardhat
Error HH18: You installed Hardhat with a corrupted lockfile due to the NPM bug #4828.

Please delete your node_modules, package-lock.json, reinstall your project, and try again.

[baumstern]

Great job on this PR! Really appreciate all the effort we put into this.

[ctrlc03]

Great job on this PR! Really appreciate all the effort we put into this.

thanks for the time taken to review this big one!