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

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .github/workflows/contracts-ci.yaml
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
name: Contracts CI

on:
push:
paths:
- 'packages/contracts/**/*.sol'
pull_request:
paths:
- 'packages/contracts/**/*.sol'

jobs:
format:
contracts:
runs-on: ubuntu-latest
defaults:
run:
Expand All @@ -21,3 +24,6 @@ jobs:
- name: Check Solidity formatting
id: format
run: make format_check

- name: Execute Unit Test
run: make test
3 changes: 3 additions & 0 deletions packages/contracts/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ format:
format_check:
forge fmt --check --root $(ROOT_DIR)

.PHONY: test
test:
forge test --root $(ROOT_DIR)

setup:
node ./script/circom-verifier-setup.js
4 changes: 2 additions & 2 deletions packages/contracts/src/managers/ZKEIP1271Manager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ abstract contract ZKEIP1271Manager is ZKAuth, ZKOwnerManager {
* @param signature Signature of the data
* @return magicValue Magic value if the signature is valid or invalid id / invalid time range
*/
function isValidSignature(bytes32 hash, bytes calldata signature) external view returns (bytes4 magicValue) {
function isValidSignature(bytes32 hash, bytes calldata signature) external pure returns (bytes4 magicValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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;
    }
  }

// todo
(hash, signature);
return _INVALID_ID;
Expand All @@ -36,7 +36,7 @@ abstract contract ZKEIP1271Manager is ZKAuth, ZKOwnerManager {
*/
function _isValidSignature(bytes32 hash, bytes calldata signature)
internal
view
pure
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

returns (uint256 validationData, bool isValid)
{
// todo
Expand Down