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

chore: Fix zkforge script support #211

Merged
merged 14 commits into from
Jan 18, 2024
Merged

Conversation

MexicanAce
Copy link
Collaborator

@MexicanAce MexicanAce commented Dec 21, 2023

What πŸ’»

  • Fix zkforge script for files in script/ dir

Why βœ‹

  • So that we can use it for deployments/etc.

Evidence πŸ“·

The current evidence for these changes include the following steps:

  1. Build latest of this repo with make build-zkforge
  2. Clone the zkfoundry example repo
  3. cd min-example-zkfoundry
  4. Run zkforge install to download the various deps
  5. Test CounterScript with:
zkforge script script/Counter.s.sol:CounterScript --chain 260 --use 0.8.21
  1. Test MyScript with:
zkforge script script/NFT.s.sol:MyScript --chain 260 --use 0.8.21

Notes πŸ“

  • This branch is currently failing for any script that's using a cheatcode, e.g.CounterScript. I've left comments in the branch with where we probably need to update the use of executor.deploy to run an era call/transaction (it's a call for this case as zkforge script is not committing the call
Screenshot 2024-01-09 at 2 11 10β€―AM

@Karrq Karrq self-assigned this Jan 8, 2024
@Karrq Karrq force-pushed the nmv/add-zkforge-script-support branch from adfe1cf to 8e6c0a9 Compare January 8, 2024 18:01
@Karrq
Copy link
Contributor

Karrq commented Jan 8, 2024

Rebased on main - scripts now work thanks to #222 :)

TODO: retrieve broadcastable txs from #201

@Karrq
Copy link
Contributor

Karrq commented Jan 9, 2024

Added support to retrieve broadcastable transactions from execution.

To test it, it's necessary to specify the rpc via --rpc flag to zkforge script, as well as a wallet(s) matching the key(s) broadcasting the transactions.
Screenshot 2024-01-10 at 5 42 59β€―AM

@Karrq Karrq marked this pull request as ready for review January 9, 2024 21:45
Copy link
Collaborator

@dutterbutter dutterbutter left a comment

Choose a reason for hiding this comment

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

@Karrq I think some of the left over work / debugging is still present and needs to be updated to make use of the changes in main. Particularly around zk_compile, and build.rs.

crates/common/src/zk_compile.rs Outdated Show resolved Hide resolved
crates/common/src/zk_compile.rs Outdated Show resolved Hide resolved
crates/zkforge/bin/cmd/script/build.rs Show resolved Hide resolved
crates/zkforge/bin/cmd/script/runner.rs Outdated Show resolved Hide resolved
crates/zkforge/bin/cmd/test/mod.rs Show resolved Hide resolved
crates/zkforge/bin/cmd/zk_build.rs Show resolved Hide resolved
@Karrq Karrq force-pushed the nmv/add-zkforge-script-support branch from 6055ee2 to c9ef504 Compare January 11, 2024 13:49
Copy link
Contributor

@Karrq Karrq left a comment

Choose a reason for hiding this comment

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

@dutterbutter I updated the PR based on your comments and replied wrt the async zksolc setup

crates/evm/core/src/era_revm/transactions.rs Show resolved Hide resolved
crates/zkforge/bin/cmd/script/build.rs Show resolved Hide resolved
@dutterbutter
Copy link
Collaborator

Assuming dependent on #230 @Karrq ?

@Karrq
Copy link
Contributor

Karrq commented Jan 16, 2024

@dutterbutter I think we should look into merging this PR first, since the scripting itself is working.
Could you also better define for us what are you requirements for scripting? What I mean is that broadcasting (and thus deployment) is part of scripting but not required for it to work (but I do understand that scripting alone is useless).
This is so we can work on them next as separate PRs.

/cc @HermanObst

@dutterbutter
Copy link
Collaborator

@Karrq I think merging this in is fine and proceeding with fixing to include deployment scripts as discussed. Please address the comment regarding the default values.

@Karrq Karrq merged commit 2ddfbb2 into main Jan 18, 2024
10 checks passed
@Karrq Karrq deleted the nmv/add-zkforge-script-support branch January 18, 2024 12:10
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.

3 participants