Add Unit Test CI to packages/contracts for Early Detection of Code Breakages

[sakuyacatcat]

Description

In this pull request, we are adding a step to run forge test -vvv in the CI for packages/contracts, as introduced in the #33 pull request.

This addition helps prevent situations where changes are merged into branches like 'develop' without running tests during development, potentially causing code breakages that could impact stakeholders and many developers.

Key Changes

• Addition of forge test to Makefile
  • We added forge test -vvv to the 'test' target in the Makefile.
• Adding Push Trigger and Test Step to CI Workflow
  • In addition to the trigger from the Implement caching and update formatting checks in CI #33 pull request, we've configured the CI workflow to execute when there are changes to .sol code in packages/contracts due to a push event. We've also added the test step, ensuring that tests run every time the CI workflow is triggered.

These changes will contribute to a more stable development process and help establish clear guidelines for our development efforts.

Other Changes

While running the compilation, I also made changes to the visibility modifiers of packages/contracts/src/managers/ZKEIP1271Manager.sol to address the warnings.

[motemotech]

This specification is defined in EIP1271 and this should be a view function.

[sakuyacatcat]

Thank you for the information.

This specification is defined in EIP1271 and this should be a view function.

I referred to this: https://eips.ethereum.org/EIPS/eip-1271

I made these modifications following the Warning messages that were output when I compiled the code in my local environment.

> forge build
[⠘] Compiling...
[⠰] Compiling 2 files with 0.8.20
[⠔] Solc 0.8.20 finished in 28.71s
Compiler run successful with warnings:
Warning (2018): Function state mutability can be restricted to pure
  --> src/managers/ZKEIP1271Manager.sol:24:5:
   |
24 |     function isValidSignature(bytes32 hash, bytes calldata signature) external view returns (bytes4 magicValue) {
   |     ^ (Relevant source part starts here and spans across multiple lines).

Regarding the choice of visibility modifiers, I proposed the use of pure since the current code does not access any state variables. However, I will adhere to the proposal in EIP-1271.

[sakuyacatcat]

To clarify my understanding, is the following code intended for the todo section? This code is from the EIP-1271 proposal.

  /**
   * @notice Verifies that the signer is the owner of the signing contract.
   */
  function isValidSignature(
    bytes32 _hash,
    bytes calldata _signature
  ) external override view returns (bytes4) {
    // Validate signatures
    if (recoverSigner(_hash, _signature) == owner) {
      return 0x1626ba7e;
    } else {
      return 0xffffffff;
    }
  }

[motemotech]

This function is not actually defined in EIP.
Could you pls let me know why did you chose pure not view

[sakuyacatcat]

I made these modifications following the Warning messages that were output when I compiled the code in my local environment.

> forge build
[⠘] Compiling...
[⠰] Compiling 2 files with 0.8.20
[⠔] Solc 0.8.20 finished in 28.71s
Compiler run successful with warnings:
Warning (2018): Function state mutability can be restricted to pure
  --> src/managers/ZKEIP1271Manager.sol:37:5:
   |
37 |     function _isValidSignature(bytes32 hash, bytes calldata signature)
   |     ^ (Relevant source part starts here and spans across multiple lines).

Regarding the choice of visibility modifiers, I considered pure to be appropriate since the current code does not access any state variables in contract.

If it's anticipated that the todo sections you mentioned in the comments will involve accessing state variables, then it might be acceptable to ignore this warning. How do you think about it?

[motemotech]

@sakuyacatcat
Sry for being late!
Thank you so much for your contribution!