-
Notifications
You must be signed in to change notification settings - Fork 227
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 CLI Foundry Template #854
Conversation
v1.9.2
v4.0.3
v4.0.3
v4.0.3
v4.0.3
v4.0.3
…s and private key
…rifier addresses for test chain
New branch
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
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.
Thanks @timou0911 that's a great start, I could successfully test executing some of the tasks (make {build,test,coverage,gas-report...}
).
I think there is still some work left to do (especially here).
The CLI template should become an npm package that we will publish so it can be selected as template when using the create
command of the CLI.
.
Here is for instance a PR that added a previous template.
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.
I personally love Makefile
as a task runner.
But I am wondering if it is appropriate for this repo.
This repo is very "node ecosystem" oriented. So we run tasks with yarn
.
On one side I agree it may not make much sense to use yarn
as a task runner for a template that does not include any js,ts
files, making make
a good choice.
One the other side, for the sake of consistency maybe we should stick to yarn
: meaning all these make tasks belong rather in package.json scripts
. And eventually we need a package.json in order to publish the template as an npm package... so...
What do you think @vplasencia @cedoor ?
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.
I agree! As soon as we have 1 non-NPM package only we should make it a NPM package. If there are many other non-NPM packages we can think of a system to download templates that is more language-agnostic.
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.
Good question. Regarding if it's better to use Makefile or yarn, I think it depends on what developers prefer when using Foundry. If most devs prefer to use Makefile, we should provide the Foundry template with Makefile and if they prefer yarn (npm, pnpm) we should provide the Foundry template with yarn. If there are no strong opinions on that we could decide one.
If we have to decide one, I see pros and cons. I think for just contracts, Makefile could be lighter and easier to add but for bigger projects yarn (npm, pnpm) could be better.
I have seen projects using both, so another alternative to discuss could be if it's better to add both.
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.
What about having Makefile
as a task runner and a package.json
to make it compatible/downloadable with our CLI?
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.
We could also add a package.json
to download the template and then remove it after it's downloaded, the same way we create the yarn.lock
file but removing a file instead of creating it.
Or if we keep the package.json
and add the scripts so that people decide the one they prefer and remove the Makefile or the package.json
and yarn.lock
files.
What do you think?
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.
I tend to prefer the first option.
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.
Yes let's go with the option of keeping the Makefile
and removing the package.json
after it having been downloaded. (That's was the 1st option right?)
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.
@timou0911 @sripwoud Is this PR ready to be merged?
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.
Sorry, I was busy in the past few weeks. I will start handling it next week.
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.
No problem!
@timou0911 @csiejimmyliu dear Taiwan fellows, may I pick up this PR and continue where it left off? |
Sure. I apologize for not being able to complete this issue sooner due to my busy schedule.
|
Hi @jimmychu0807, be sure to create a branch from this PR, so that we can merge it here. |
I tried to do this, and ended up with a new PR #905. If this is important to fix, could you point me to the doc? Thanks. |
@jimmychu0807 That PR is fine, thanks! |
* Semaphore identity example code bug fix * Receive suggestion for consistency * chore: forge init * forge install: forge-std v1.9.2 * Foundry CLI First Draft * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * forge install: forge-std v1.9.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * forge install: forge-std v1.9.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * forge install: forge-std v1.9.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * forge install: forge-std v1.9.2 * modules * forge install: semaphore v4.0.3 * forge install: zk-kit.solidity * forge install: poseidon-solidity v0.0.5 * forge install: openzeppelin-contracts v5.0.2 * forge install: forge-std v1.9.2 * change test name * modify declaration of semaphore and verifier * Modify Test Function Name * Add Test Chain Target * forge std install * refactor(cli-template-contracts-foundry): change default Anvil address and private key * chore(cli-template-contracts-foundry): add comments * refactor(cli-template-contracts-foundry): add Semaphore & SemaphoreVerifier addresses for test chain * chore(cli-template-contracts-foundry): add forge coverage for Makefile * chore(cli-template-contracts-foundry): add env.example * docs(cli-template-contracts-foundry): add command instructions * updated * forge build works * Fixed for linting * chore(cli-template-contracts-foundry): make the lint, prettier, and lint-staged pass * chore(cli-template-contracts-foundry): replace Makefile(removed) with package.json * chore(cli-template-contracts-foundry): passing the ci test * updated test * feat(cli-template-contracts-foundry): complete cli-template-contracts-foundry re #854, #185 * Update dependencies * Add explanation on `yarn dev` * fix(cli-template-contracts-foundry): fix `yarn dev` command and add docs on integrate w/ boilerplate * Added yarnrc * updated version * Added Foundry in template option --------- Co-authored-by: weipooppys93030 <55434365+weipooppys93030@users.noreply.github.com> Co-authored-by: timou0911 <x0928048316@gmail.com> Co-authored-by: csiejimmyliu <91661606+csiejimmyliu@users.noreply.github.com>
Closing this PR as it was continued and completed here: #905 Thank you very much @timou0911 for the great work ✨ @gitpoap-bot @timou0911 |
Congrats, @timou0911 ! You've earned a GitPOAP for your contribution! GitPOAP: 2024 Semaphore Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
Description
Create a CLI foundry template with a sample contract, a deploy script, and tests for it.
The contract itself is the same as in CLI hardhat template.
Related Issue(s)
Closes #185
Checklist
yarn format
andyarn lint
without getting any errors