From cf0a88d18218471dcf13d62afa4f8fe5335d9740 Mon Sep 17 00:00:00 2001 From: Juan Rigada <62958725+Jrigada@users.noreply.github.com> Date: Fri, 24 Jan 2025 13:54:47 -0300 Subject: [PATCH] fix: Remove wrong estimation in create for zksync transactions (#864) * Remove wrong estimation in create for zksync transactions, add timeout parameter being retrieve wrong and gas per pubdata parameter * Fix estimation on non legacy chains for scripts * Format --------- Co-authored-by: Dustin Brickwood Co-authored-by: Nisheeth Barthwal --- crates/forge/bin/cmd/create.rs | 35 +++++++---------- crates/forge/tests/it/test_helpers.rs | 5 +++ crates/forge/tests/it/zk/create.rs | 56 ++++++++++++++++++++++++++- crates/forge/tests/it/zk/linking.rs | 1 + crates/script/src/broadcast.rs | 26 +++++++++---- 5 files changed, 92 insertions(+), 31 deletions(-) diff --git a/crates/forge/bin/cmd/create.rs b/crates/forge/bin/cmd/create.rs index c8d4a4ddf..d15a88f8f 100644 --- a/crates/forge/bin/cmd/create.rs +++ b/crates/forge/bin/cmd/create.rs @@ -113,6 +113,10 @@ pub struct CreateArgs { #[command(flatten)] retry: RetryArgs, + + /// Gas per pubdata + #[clap(long = "zk-gas-per-pubdata", value_name = "GAS_PER_PUBDATA")] + pub zk_gas_per_pubdata: Option, } #[derive(Debug, Default)] @@ -128,7 +132,7 @@ impl CreateArgs { /// Executes the command to create a contract pub async fn run(mut self) -> Result<()> { let mut config = self.try_load_config_emit_warnings()?; - + let timeout = config.transaction_timeout; // Install missing dependencies. if install::install_missing_dependencies(&mut config) && config.auto_detect_remappings { // need to re-configure here to also catch additional remappings @@ -301,7 +305,7 @@ impl CreateArgs { provider, chain_id, deployer, - config.transaction_timeout, + timeout, id, zk_data, ) @@ -659,7 +663,6 @@ impl CreateArgs { e } })?; - let is_legacy = self.tx.legacy || Chain::from(chain).is_legacy(); deployer.tx = deployer.tx.with_factory_deps( zk_data.factory_deps.clone().into_iter().map(|dep| dep.into()).collect(), @@ -692,24 +695,13 @@ impl CreateArgs { deployer.tx.set_gas_price(gas_price); // estimate fee - foundry_zksync_core::estimate_fee(&mut deployer.tx, &provider, 130, None).await?; - - if !is_legacy { - let estimate = provider.estimate_eip1559_fees(None).await.wrap_err("Failed to estimate EIP1559 fees. This chain might not support EIP1559, try adding --legacy to your command.")?; - let priority_fee = if let Some(priority_fee) = self.tx.priority_gas_price { - priority_fee.to() - } else { - estimate.max_priority_fee_per_gas - }; - let max_fee = if let Some(max_fee) = self.tx.gas_price { - max_fee.to() - } else { - estimate.max_fee_per_gas - }; - - deployer.tx.set_max_fee_per_gas(max_fee); - deployer.tx.set_max_priority_fee_per_gas(priority_fee); - } + foundry_zksync_core::estimate_fee( + &mut deployer.tx, + &provider, + 130, + self.zk_gas_per_pubdata, + ) + .await?; if let Some(gas_limit) = self.tx.gas_limit { deployer.tx.set_gas_limit(gas_limit.to::()); @@ -989,6 +981,7 @@ where .send_transaction(self.tx) .await? .with_required_confirmations(self.confs as u64) + .with_timeout(Some(std::time::Duration::from_secs(self.timeout))) .get_receipt() .await?; diff --git a/crates/forge/tests/it/test_helpers.rs b/crates/forge/tests/it/test_helpers.rs index a9d34e679..f05855ffa 100644 --- a/crates/forge/tests/it/test_helpers.rs +++ b/crates/forge/tests/it/test_helpers.rs @@ -624,6 +624,7 @@ pub fn deploy_zk_contract( url: &str, private_key: &str, contract_path: &str, + extra_args: Option<&[&str]>, ) -> Result { cmd.forge_fuse().args([ "create", @@ -635,6 +636,10 @@ pub fn deploy_zk_contract( private_key, ]); + if let Some(args) = extra_args { + cmd.args(args); + } + let output = cmd.assert_success(); let output = output.get_output(); let stdout = output.stdout_lossy(); diff --git a/crates/forge/tests/it/zk/create.rs b/crates/forge/tests/it/zk/create.rs index 8d9af9d2e..84ea72092 100644 --- a/crates/forge/tests/it/zk/create.rs +++ b/crates/forge/tests/it/zk/create.rs @@ -17,7 +17,7 @@ forgetest_async!(forge_zk_can_deploy_erc20, |prj, cmd| { ZkSyncNode::rich_wallets().next().map(|(_, pk, _)| pk).expect("No rich wallets available"); let erc20_address = - deploy_zk_contract(&mut cmd, url.as_str(), private_key, "./src/ERC20.sol:MyToken") + deploy_zk_contract(&mut cmd, url.as_str(), private_key, "./src/ERC20.sol:MyToken", None) .expect("Failed to deploy ERC20 contract"); assert!(!erc20_address.is_empty(), "Deployed address should not be empty"); @@ -43,10 +43,11 @@ forgetest_async!(forge_zk_can_deploy_contracts_and_cast_a_transaction, |prj, cmd url.as_str(), private_key, "./src/TokenReceiver.sol:TokenReceiver", + None, ) .expect("Failed to deploy TokenReceiver contract"); let erc_20_address = - deploy_zk_contract(&mut cmd, url.as_str(), private_key, "./src/ERC20.sol:MyToken") + deploy_zk_contract(&mut cmd, url.as_str(), private_key, "./src/ERC20.sol:MyToken", None) .expect("Failed to deploy ERC20 contract"); cmd.cast_fuse().args([ @@ -66,3 +67,54 @@ forgetest_async!(forge_zk_can_deploy_contracts_and_cast_a_transaction, |prj, cmd assert!(stdout.contains("transactionHash"), "Transaction hash not found in output"); assert!(stdout.contains("success"), "Transaction was not successful"); }); + +forgetest_async!(forge_zk_can_deploy_contracts_with_gas_per_pubdata_and_succeed, |prj, cmd| { + util::initialize(prj.root()); + prj.add_source("ERC20.sol", include_str!("../../../../../testdata/zk/ERC20.sol")).unwrap(); + + let node = ZkSyncNode::start().await; + let url = node.url(); + + let private_key = + ZkSyncNode::rich_wallets().next().map(|(_, pk, _)| pk).expect("No rich wallets available"); + + deploy_zk_contract( + &mut cmd, + url.as_str(), + private_key, + "./src/ERC20.sol:MyToken", + Some(&["--zk-gas-per-pubdata", "3000"]), + ) + .expect("Failed to deploy ERC20 contract"); +}); + +forgetest_async!( + forge_zk_can_deploy_contracts_with_invalid_gas_per_pubdata_and_fail, + |prj, cmd| { + util::initialize(prj.root()); + prj.add_source("ERC20.sol", include_str!("../../../../../testdata/zk/ERC20.sol")).unwrap(); + + let node = ZkSyncNode::start().await; + let url = node.url(); + + let private_key = ZkSyncNode::rich_wallets() + .next() + .map(|(_, pk, _)| pk) + .expect("No rich wallets available"); + cmd.forge_fuse().args([ + "create", + "--zk-startup", + "./src/ERC20.sol:MyToken", + "--rpc-url", + url.as_str(), + "--private-key", + private_key, + "--timeout", + "1", + "--zk-gas-per-pubdata", + "1", + ]); + + cmd.assert_failure(); + } +); diff --git a/crates/forge/tests/it/zk/linking.rs b/crates/forge/tests/it/zk/linking.rs index 4bdd728b7..76758efcd 100644 --- a/crates/forge/tests/it/zk/linking.rs +++ b/crates/forge/tests/it/zk/linking.rs @@ -37,6 +37,7 @@ forgetest_async!( "127.0.0.1:1234", "0x0000000000000000000000000000000000000000000000000000000000000000", "./src/WithLibraries.sol:UsesFoo", + None, ) .expect("Failed to deploy UsesFoo contract"); diff --git a/crates/script/src/broadcast.rs b/crates/script/src/broadcast.rs index 680d13163..809a3dd92 100644 --- a/crates/script/src/broadcast.rs +++ b/crates/script/src/broadcast.rs @@ -348,17 +348,27 @@ impl BundledState { }), ), (false, _, _) => { - let mut fees = provider.estimate_eip1559_fees(None).await.wrap_err("Failed to estimate EIP1559 fees. This chain might not support EIP1559, try adding --legacy to your command.")?; + if self.script_config.config.zksync.run_in_zk_mode() { + // NOTE(zk): We need to avoid estimating eip1559 fees for zk + // transactions as the fee is estimated + // later. This branch is for non legacy chains (zkchains). + (Some(provider.get_gas_price().await?), None) + } else { + let mut fees = provider + .estimate_eip1559_fees(None) + .await + .wrap_err("Failed to estimate EIP1559 fees. This chain might not support EIP1559, try adding --legacy to your command.")?; - if let Some(gas_price) = self.args.with_gas_price { - fees.max_fee_per_gas = gas_price.to(); - } + if let Some(gas_price) = self.args.with_gas_price { + fees.max_fee_per_gas = gas_price.to(); + } - if let Some(priority_gas_price) = self.args.priority_gas_price { - fees.max_priority_fee_per_gas = priority_gas_price.to(); - } + if let Some(priority_gas_price) = self.args.priority_gas_price { + fees.max_priority_fee_per_gas = priority_gas_price.to(); + } - (None, Some(fees)) + (None, Some(fees)) + } } };