-
Notifications
You must be signed in to change notification settings - Fork 6
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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" |
There was a problem hiding this comment.
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).
[[bin]] | ||
name = "generate" | ||
path = "src/main.rs" |
There was a problem hiding this comment.
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.
handlebars = "*" | ||
serde = "*" | ||
serde_derive = "*" | ||
serde_json = "*" | ||
hex = "*" |
There was a problem hiding this comment.
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.
rescue_poseidon = {package = "rescue_poseidon", path = "../rescue-poseidon"} | ||
# rescue-poseidon = {package = "rescue_poseidon", git = "https://github.com/matter-labs/rescue-poseidon"} |
There was a problem hiding this comment.
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/ |
There was a problem hiding this comment.
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?
# <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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# <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). | |
## 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); | ||
} | ||
} | ||
``` |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
Replace hardhat testing tool with foundry and fix tests for various circuit types.