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: fetch all factory deps for a given contract #316

Merged
merged 27 commits into from
Apr 18, 2024

Conversation

Karrq
Copy link
Contributor

@Karrq Karrq commented Apr 11, 2024

Motivation

Currently deploying factory-style contracts requires specifying almost all dependencies manually (via something like --factory-deps).
Addressing this issue would resolve #295 as well.

Solution

Use the factory deps information provided by the compiler and bundle them in the transactions when creating contracts with factory dependencies, also performing a search to find any nested dependencies, since the compiler only returns the first level of dependencies.

Notes

The following tests fail:

crates/zksync/core/src/vm/runner.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/create.rs Show resolved Hide resolved
crates/zksync/compiler/src/zksolc/mod.rs Outdated Show resolved Hide resolved
crates/zksync/compiler/src/zksolc/mod.rs Outdated Show resolved Hide resolved
fix(test:zk): proper user factory
feat(zk:create): lookup all factory deps of manually specified deps
feat(test:zk): add scripts for factory tests
fix(test:zk): comment out non working test & add FIXME
@Karrq Karrq marked this pull request as ready for review April 15, 2024 14:14
@Karrq Karrq requested a review from nbaztec April 15, 2024 14:29
@HermanObst
Copy link
Contributor

HermanObst commented Apr 15, 2024

Lets wait to adding aave-delivery test into the CI before merging this PR. Today should be added :)
Just want to have that to ensure we are not facing any regression.

crates/zksync/compiler/src/zksolc/mod.rs Outdated Show resolved Hide resolved
crates/zksync/compiler/src/zksolc/mod.rs Outdated Show resolved Hide resolved
crates/zksync/compiler/src/zksolc/mod.rs Outdated Show resolved Hide resolved
crates/zksync/compiler/src/zksolc/mod.rs Outdated Show resolved Hide resolved
zk-tests/src/Factory.t.sol Outdated Show resolved Hide resolved
crates/zksync/compiler/src/zksolc/mod.rs Show resolved Hide resolved
crates/forge/bin/cmd/create.rs Outdated Show resolved Hide resolved
HermanObst and others added 4 commits April 15, 2024 19:02
Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com>
fix(tests:zk): enable `testUserFactory`
refactor(compile:zk): `fetch_all_factory_deps` return `Vec<Vec<u8>>`
crates/cheatcodes/src/fs.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/test.rs Show resolved Hide resolved
crates/cheatcodes/src/utils.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/create.rs Outdated Show resolved Hide resolved
@Karrq Karrq requested a review from nbaztec April 16, 2024 20:38
Copy link
Collaborator

@nbaztec nbaztec left a comment

Choose a reason for hiding this comment

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

love the tests ❤️

crates/cheatcodes/src/fs.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved
crates/zksync/core/src/vm/storage_view.rs Outdated Show resolved Hide resolved
let persisted_factory_deps = self.persisted_factory_deps.clone();
// and extend it for future calls
self.persisted_factory_deps
.extend(factory_deps.iter().map(|dep| (hash_bytecode(dep), dep.clone())));
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a follow up, we should try to remove this part, and perhaps extend it only with the bytecodes returned from the era vm. So in theory, every known bytecode will reside here. Or perhaps we can also think if it's easier to support it directly in the Backend and via some changes to DatabaseExt to actually store the factory_deps directly in the storage. But that all can be a later refactor.

Copy link
Collaborator

@nbaztec nbaztec left a comment

Choose a reason for hiding this comment

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

minor doc related changes otherwise LGTM

Karrq and others added 2 commits April 18, 2024 09:35
@HermanObst HermanObst merged commit 1e5e102 into dev Apr 18, 2024
10 checks passed
@HermanObst HermanObst deleted the fix/nested-factory-deps branch April 18, 2024 12:12
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.

Missing factory dependencies during compilation causes revert: The code hash is not known error
4 participants