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

refactor: prepare migration to forge #32

Open
wants to merge 30 commits into
base: forge-dev
Choose a base branch
from
Open

Conversation

sripwoud
Copy link
Member

@sripwoud sripwoud commented Jul 10, 2024

Closes #30

This prepares the migration to forge as solidity development toolchain.
This PR makes the following changes:

  • remove all node/typescript/hardhat related deps that aren't used anymore
  • change project layout: lift up packages one level up
  • update ci for publishing:
    • keep publishing contracts as npm packages so devs can install them from a node package registry.
    • in addition push contracts to a "cleaned" forge branch for installation with forge: forge install privacy-scaling-explorations/zk-kit.solidity@forge. As forge install uses submodules and all our contracts are in the same repo, it is not possible to selectively install a zk kit contract package to install eg only excubiae with forge install, the whole repo or a single branch of the repo has to be cloned.
    • rewrite tests for lean-imt as a start: I suggest to let package authors rewrite their tests themselves later (see dx(excubiae): migrate to forge #26 dx(imt): migrate to forge #27 dx(lazy-imt): migrate to forge #28 dx(lazytower): migrate to forge #29)

@sripwoud sripwoud changed the title refactor(lean-imt): rewrite tests in sol refactor: prepare migration to forge Jul 10, 2024
@sripwoud sripwoud force-pushed the refactor/forge branch 8 times, most recently from 18fe37f to 81d050b Compare July 10, 2024 09:39
ci: restore deps in compile job

ci: restore node_modules in tests job

chore: format

ci: debug

ci: fix output name in if check

ci: debug

ci: debug

ci: fix coveralls step

chore: remove debug

test: exclude some packages from coverage

ci: debug

ci: have slither job wait for compile

ci: debug

ci: debug
Publish pkg to npm and to a branch for use by `forge install`

re #11, #30
@sripwoud sripwoud force-pushed the refactor/forge branch 2 times, most recently from 0c294bd to 45dbcc1 Compare July 10, 2024 14:56
@sripwoud sripwoud force-pushed the refactor/forge branch 2 times, most recently from 04999d9 to 9ffc7e4 Compare July 10, 2024 21:18
@sripwoud sripwoud force-pushed the refactor/forge branch 2 times, most recently from 5305e64 to 13c23df Compare July 11, 2024 07:31
@sripwoud sripwoud marked this pull request as ready for review July 11, 2024 07:40
@sripwoud sripwoud marked this pull request as draft July 11, 2024 07:46
@sripwoud sripwoud marked this pull request as ready for review July 11, 2024 08:08
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
"format": "prettier -c .",
"format:write": "prettier -w .",
"remove:stable-version-field": "ts-node scripts/remove-stable-version-field.ts ${0} && yarn format:write",
"test": "forge test",
Copy link
Member Author

Choose a reason for hiding this comment

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

i decided to remove remove:stable-version-field and version:publish because this is handled by the ci, I think it is better not to easily "expose" such scripts in package.json

"format:write": "prettier -w .",
"remove:stable-version-field": "ts-node scripts/remove-stable-version-field.ts ${0} && yarn format:write",
"test": "forge test",
"version:bump": "scripts/version-bump.sh",
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to make this a sh script because the repo does not use any typescript dev deps anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

coveralls report: coverage not 100%, I am especially not covering the revert on WrongSiblingNodes (didn't figure out how to write a corresponding test 😅

I am not sure why the coveralls bot does not write comments in the PR btw.

Comment on lines +5 to +6
# TODO: update, temporarily ignoring all except lean-imt
no_match_coverage = "^packages/(excubiae|imt|lazy-imt|lazytower)"
Copy link
Member Author

Choose a reason for hiding this comment

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

update this as we rewrite more tests in solidity


rm -fr "packages"

git commit -am "$latest_main_commit_msg"
Copy link
Member Author

Choose a reason for hiding this comment

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

this script is executed in the release workflow...
which is triggered on tags push events on main...
Tags are created with yarn version:bump...
Which always creates a chore(pkg): v(version) commit...

So on the forge branch we will only have a clean succession of chore(pkg): v(version) commit messages that mirrors our npm release workflow, making it easy for devs to checkout/select a specific contracts package version even if they install with forge install

@sripwoud sripwoud self-assigned this Jul 11, 2024
@sripwoud sripwoud linked an issue Jul 11, 2024 that may be closed by this pull request
@sripwoud sripwoud added dependencies 📦 Improvements or additions to documentation dx 🔧 Developer experience devops 🔧 Operations management and dev tools documentation 📖 Improvements or additions to documentation labels Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 📦 Improvements or additions to documentation devops 🔧 Operations management and dev tools documentation 📖 Improvements or additions to documentation dx 🔧 Developer experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dx(lean-imt): migrate to forge
3 participants