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: create in upstream merge #462

Merged
merged 17 commits into from
Jul 10, 2024
Merged

Conversation

nbaztec
Copy link
Collaborator

@nbaztec nbaztec commented Jul 8, 2024

Motivation

fixes create

Solution

Karrq and others added 8 commits July 1, 2024 20:03
@nbaztec nbaztec marked this pull request as draft July 8, 2024 14:12
@nbaztec nbaztec marked this pull request as ready for review July 9, 2024 15:01
/// Given a `Project`'s output, removes the matching ABI, Bytecode and
/// Runtime Bytecode of the given contract.
#[track_caller]
pub fn remove_zk_contract(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move zk related helpers to a respective module/crate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can refactor this once we have a better picture.
In general it's easier to have everything in one place, but we need to plan it properly as most times the "zk" functions are a copy-paste of the original ones with a few modifications. So when a upstream merge comes, we want to include those updates to the original function.

crates/forge/bin/cmd/create.rs Outdated Show resolved Hide resolved
impl CreateArgs {
/// Executes the command to create a contract
pub async fn run(mut self) -> Result<()> {
// Find Project & Compile
let project = self.opts.project()?;
let zksync = self.opts.compiler.zk.compile.unwrap_or_default();
if zksync {
let target_path = if let Some(ref mut path) = self.contract.path {
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole flow + helpers might also fit on a zksync/module crate as a function, that would make it cleaner for merge conflicts as well

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.

I think we'll need to follow this up with unification of the zk vs evm contracts like we had before (avoiding basically 2 different create depending if we are in zksync mode or not)

crates/forge/bin/cmd/create.rs Outdated Show resolved Hide resolved
crates/script/src/transaction.rs Show resolved Hide resolved
@@ -67,43 +73,54 @@ impl ZkTransactionMetadata {
}

/// Creates a new signed EIP-712 transaction with the provided factory deps.
pub async fn new_eip712_transaction<M: Middleware, S: Signer>(
legacy_or_1559: TypedTransaction,
pub async fn new_eip712_transaction<
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this (almost) what's present in script/src/broadcast.rs? I think it makes sense to (at least partially) unify this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can do the refactor once we have a better picture.

crates/zksync/core/src/lib.rs Outdated Show resolved Hide resolved
crates/zksync/core/src/lib.rs Outdated Show resolved Hide resolved
crates/zksync/core/src/lib.rs Outdated Show resolved Hide resolved
@nbaztec nbaztec merged commit 233707f into foundry-update-4af6cfa Jul 10, 2024
3 checks passed
@nbaztec nbaztec deleted the nish-fix-upstream-create branch July 10, 2024 14:41
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