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

fix: forge build, forge create, forge script and smoke tests #299

Merged
merged 16 commits into from
Apr 4, 2024

Conversation

nbaztec
Copy link
Collaborator

@nbaztec nbaztec commented Apr 2, 2024

Motivation

The following bugs were reported (and are fixed in this PR)

  • forge build --zksync this does not compile using zksolc
  • forge create src/Counter.sol:Counter --rpc-url sepolia --chain 300 --private-key <PRIVATE-KEY> --zksync - this does deploy a contract correctly to zksync.
    Error (code: 3, message: Failed to serialize transaction: toAddressIsNull, data: None)
  • forge script script/Counter.s.sol:CounterScript --rpc-url=https://sepolia.era.zksync.dev/ --broadcast --private-key 0x... --zksync
    Error: no zk contract was found for 0x...

Solution

  • Ports forge test functionality to forge build, forge create and forge script.
  • Refactors ZkSolc configuration to use the builder pattern.
  • ZK mode can be activated either via vm.zkVm(true) cheatcode or passing the --zksync flag.
  • Adds smoke tests.
  • Splits foundry_zksync into foundry_zksync_core and foundry_zksync_compiler crates to avoid circular dependecies.

@Karrq Karrq requested review from Karrq and Jrigada April 3, 2024 16:26
@nbaztec nbaztec force-pushed the nish-fix-build branch 2 times, most recently from 7b0a68e to deac01b Compare April 4, 2024 13:15
Copy link
Contributor

@Jrigada Jrigada left a comment

Choose a reason for hiding this comment

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

This is amazing 🚀

@nbaztec nbaztec merged commit 871261b into dev Apr 4, 2024
8 of 9 checks passed
@nbaztec nbaztec deleted the nish-fix-build branch April 4, 2024 16:22
@@ -3,10 +3,10 @@ name: test
on:
push:
branches:
- foundry/main
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: any reason why not both?

TransactTo::Create(CreateScheme::Create2 { .. }) => CONTRACT_DEPLOYER_ADDRESS,
TransactTo::Create(CreateScheme::Create2 { .. }) => (
CONTRACT_DEPLOYER_ADDRESS,
ZKVMData::new(db, &mut journaled_state).get_deploy_nonce(caller),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this deviates from hardhat - I tried searching the zksync hardhart plugin to understand what nonce they use for deployments, and it seems they always use the tx nonces.
The deployment nonce is handled internally by the system contracts.
After all a deployment is still a transaction.

Also I think we don't use this nonce everywhere for deployments, so it would be inconsistent (pretty sure I saw something in the files above that uses tx nonce for a create)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting, yeah the tx nonce is guaranteed to be greater than the deployment nonce. But it makes sense to pass the tx nonce, if the system contracts only ever use the deployment nonce internally. I'll investigate and update as needed.

@@ -168,18 +174,18 @@ where
let caller = call.caller;
let calldata = encode_create_params(&call.scheme, contract.zk_bytecode_hash, constructor_input);
let factory_deps = vec![contract.zk_deployed_bytecode.clone()];
let nonce = ZKVMData::new(db, journaled_state).get_tx_nonce(caller);
let nonce = ZKVMData::new(db, journaled_state).get_deploy_nonce(caller);
Copy link
Contributor

Choose a reason for hiding this comment

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

same

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