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

Replace hardhat with foundry for Plonk verifier and fix CI tests #9

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

saitima
Copy link
Member

@saitima saitima commented Aug 15, 2024

Replace hardhat testing tool with foundry and fix tests for various circuit types.

Copy link
Member

@popzxc popzxc left a comment

Choose a reason for hiding this comment

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

Thank you! Will review next week, right now will mark as "Request changes" to prevent accidental merge.

@saitima saitima changed the title Replace hardhat with foundry and fix CI tests Replace hardhat with foundry for Plonk verifier and fix CI tests Aug 16, 2024
Copy link
Member

@popzxc popzxc left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for the effort, much appreciated!
Left a few comments. If you don't have the time to work on them, please let me know and I will try to do these changes myself.

@@ -1,14 +1,17 @@
[package]
name = "codegen-bin"
name = "plonk-verifier-codegen-bin"
Copy link
Member

Choose a reason for hiding this comment

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

Could you rollback the renaming changes (here and below), please? We will rename all the crates soon, but it will be a centralized effort (otherwise it'll be hard to propagate changes through the chain of dependencies).

Comment on lines +15 to +17
[[bin]]
name = "generate"
path = "src/main.rs"
Copy link
Member

Choose a reason for hiding this comment

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

Could you roll it back as well, please? Since this crate now exists within the workspace, having a custom command is confusing -- naming the binary is more intuitive.

Comment on lines +10 to +14
handlebars = "*"
serde = "*"
serde_derive = "*"
serde_json = "*"
hex = "*"
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use wildcard dependencies -- packages with wildcard deps cannot be published on crates.io. As a rule of thumb, using major.minor is a good approach.

Comment on lines +17 to +18
rescue_poseidon = {package = "rescue_poseidon", path = "../rescue-poseidon"}
# rescue-poseidon = {package = "rescue_poseidon", git = "https://github.com/matter-labs/rescue-poseidon"}
Copy link
Member

Choose a reason for hiding this comment

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

Can be just rescue_poseidon.workspace = true.

@@ -0,0 +1,3 @@
cache/
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really belong to crates, right? Could you please create contracts/ folder and move it there?

Comment on lines +1 to +5
# <h1 align="center"> Forge Template </h1>

**Template repository for getting started quickly with Foundry projects**

![Github Actions](https://github.com/foundry-rs/forge-template/workflows/CI/badge.svg)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# <h1 align="center"> Forge Template </h1>
**Template repository for getting started quickly with Foundry projects**
![Github Actions](https://github.com/foundry-rs/forge-template/workflows/CI/badge.svg)
# Plonk Verifier Forge Template
This repository contains contracts to be used together with the output of [PLONK verifier codegen](../crates/plonk-verifier-codegen-bin).

Comment on lines +7 to +35
## Getting Started

Click "Use this template" on [GitHub](https://github.com/foundry-rs/forge-template) to create a new repository with this repo as the initial state.

Or, if your repo already exists, run:
```sh
forge init
forge build
forge test
```

## Writing your first test

All you need is to `import forge-std/Test.sol` and then inherit it from your test contract. Forge-std's Test contract comes with a pre-instatiated [cheatcodes environment](https://book.getfoundry.sh/cheatcodes/), the `vm`. It also has support for [ds-test](https://book.getfoundry.sh/reference/ds-test.html)-style logs and assertions. Finally, it supports Hardhat's [console.log](https://github.com/brockelmore/forge-std/blob/master/src/console.sol). The logging functionalities require `-vvvv`.

```solidity
pragma solidity 0.8.10;

import "forge-std/Test.sol";

contract ContractTest is Test {
function testExample() public {
vm.roll(100);
console.log(1);
emit log("hi");
assertTrue(true);
}
}
```
Copy link
Member

Choose a reason for hiding this comment

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

These lines can be removed.

@@ -0,0 +1,39 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

This can be moved to the contracts/plonk-verifier-foundry

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.

2 participants