From 7736a7228ac1a8670d36061d64ef85b29c246dcf Mon Sep 17 00:00:00 2001 From: Danil Date: Tue, 2 Jul 2024 10:13:44 +0200 Subject: [PATCH 1/5] Resume functionality Signed-off-by: Danil --- contracts | 2 +- zk_toolbox/crates/common/src/cmd.rs | 16 +++++++++--- zk_toolbox/crates/common/src/forge.rs | 35 +++++++++++++++++++++++++-- 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/contracts b/contracts index db9387690502..8172969672cc 160000 --- a/contracts +++ b/contracts @@ -1 +1 @@ -Subproject commit db9387690502937de081a959b164db5a5262ce0a +Subproject commit 8172969672cc6a38542cd8f5578c74b7e30cd3b4 diff --git a/zk_toolbox/crates/common/src/cmd.rs b/zk_toolbox/crates/common/src/cmd.rs index 0a0d936b90e7..fe35c2395336 100644 --- a/zk_toolbox/crates/common/src/cmd.rs +++ b/zk_toolbox/crates/common/src/cmd.rs @@ -1,3 +1,4 @@ +use std::fmt::{Display, Formatter}; use std::{ ffi::OsStr, io, @@ -21,10 +22,19 @@ pub struct Cmd<'a> { } #[derive(thiserror::Error, Debug)] -#[error("Cmd error: {source} {stderr:?}")] pub struct CmdError { - stderr: Option, - source: anyhow::Error, + pub stderr: Option, + pub source: anyhow::Error, +} + +impl Display for CmdError { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + let mut data = format!("{}", &self.source); + if let Some(stderr) = &self.stderr { + data = format!("{data}\n{stderr}"); + } + write!(f, "{}", data) + } } impl From for CmdError { diff --git a/zk_toolbox/crates/common/src/forge.rs b/zk_toolbox/crates/common/src/forge.rs index a858333cd2c0..af5e9ed717e7 100644 --- a/zk_toolbox/crates/common/src/forge.rs +++ b/zk_toolbox/crates/common/src/forge.rs @@ -14,6 +14,7 @@ use serde::{Deserialize, Serialize}; use strum_macros::Display; use xshell::{cmd, Shell}; +use crate::cmd::CmdError; use crate::{cmd::Cmd, ethereum::create_ethers_client}; /// Forge is a wrapper around the forge binary. @@ -54,8 +55,27 @@ impl ForgeScript { pub fn run(mut self, shell: &Shell) -> anyhow::Result<()> { let _dir_guard = shell.push_dir(&self.base_path); let script_path = self.script_path.as_os_str(); - let args = self.args.build(); - Ok(Cmd::new(cmd!(shell, "forge script {script_path} --legacy {args...}")).run()?) + let args_no_resume = self.args.build(); + if self.args.resume { + // Resume doesn't work if the forge script has never been started on this chain before. + // So we want to catch it and try again without resume arg if it's the case + let mut args = args_no_resume.clone(); + args.push(ForgeScriptArg::Resume.to_string()); + if let Err(err) = + Cmd::new(cmd!(shell, "forge script {script_path} --legacy {args...}")).run() + { + if !check_for_resume_not_successful_because_has_not_began(&err) { + return Err(err.into()); + } + } else { + return Ok(()); + }; + } + Ok(Cmd::new(cmd!( + shell, + "forge script {script_path} --legacy {args_no_resume...}" + )) + .run()?) } pub fn wallet_args_passed(&self) -> bool { @@ -208,6 +228,7 @@ pub enum ForgeScriptArg { url: String, }, Verify, + Resume, } /// ForgeScriptArgs is a set of arguments that can be passed to the forge script command. @@ -229,6 +250,8 @@ pub struct ForgeScriptArgs { /// Verifier API key #[clap(long)] pub verifier_api_key: Option, + #[clap(long)] + pub resume: bool, /// List of additional arguments that can be passed through the CLI. /// /// e.g.: `zk_inception init -a --private-key=` @@ -348,3 +371,11 @@ pub enum ForgeVerifier { Blockscout, Oklink, } + +fn check_for_resume_not_successful_because_has_not_began(error: &CmdError) -> bool { + if let Some(stderr) = &error.stderr { + stderr.contains("Deployment not found for chain") + } else { + false + } +} From 151b935d58d2d5c4d30eaaab5cf4db62ba658570 Mon Sep 17 00:00:00 2001 From: Danil Date: Tue, 2 Jul 2024 18:36:35 +0200 Subject: [PATCH 2/5] Check the error Signed-off-by: Danil --- zk_toolbox/crates/common/src/cmd.rs | 2 +- zk_toolbox/crates/common/src/forge.rs | 36 +++++++-- .../zk_inception/src/accept_ownership.rs | 73 +++++++++---------- 3 files changed, 65 insertions(+), 46 deletions(-) diff --git a/zk_toolbox/crates/common/src/cmd.rs b/zk_toolbox/crates/common/src/cmd.rs index fe35c2395336..a0a4b7d10ba9 100644 --- a/zk_toolbox/crates/common/src/cmd.rs +++ b/zk_toolbox/crates/common/src/cmd.rs @@ -1,6 +1,6 @@ -use std::fmt::{Display, Formatter}; use std::{ ffi::OsStr, + fmt::{Display, Formatter}, io, process::{Command, Output, Stdio}, string::FromUtf8Error, diff --git a/zk_toolbox/crates/common/src/forge.rs b/zk_toolbox/crates/common/src/forge.rs index af5e9ed717e7..c46a1e7c95ff 100644 --- a/zk_toolbox/crates/common/src/forge.rs +++ b/zk_toolbox/crates/common/src/forge.rs @@ -5,17 +5,20 @@ use std::{ use clap::{Parser, ValueEnum}; use ethers::{ + core::types::Bytes, middleware::Middleware, prelude::{LocalWallet, Signer}, types::{Address, H256, U256}, - utils::hex::ToHex, + utils::{hex, hex::ToHex}, }; use serde::{Deserialize, Serialize}; use strum_macros::Display; use xshell::{cmd, Shell}; -use crate::cmd::CmdError; -use crate::{cmd::Cmd, ethereum::create_ethers_client}; +use crate::{ + cmd::{Cmd, CmdError}, + ethereum::create_ethers_client, +}; /// Forge is a wrapper around the forge binary. pub struct Forge { @@ -71,11 +74,18 @@ impl ForgeScript { return Ok(()); }; } - Ok(Cmd::new(cmd!( + if let Err(err) = Cmd::new(cmd!( shell, "forge script {script_path} --legacy {args_no_resume...}" )) - .run()?) + .run() + { + if check_the_operation_proposal_error(&err) { + return Ok(()); + } + return Err(err.into()); + } + Ok(()) } pub fn wallet_args_passed(&self) -> bool { @@ -107,6 +117,13 @@ impl ForgeScript { self } + pub fn with_calldata(mut self, calldata: &Bytes) -> Self { + self.args.add_arg(ForgeScriptArg::Sig { + sig: hex::encode(calldata), + }); + self + } + /// Makes sure a transaction is sent, only after its previous one has been confirmed and succeeded. pub fn with_slow(mut self) -> Self { self.args.add_arg(ForgeScriptArg::Slow); @@ -379,3 +396,12 @@ fn check_for_resume_not_successful_because_has_not_began(error: &CmdError) -> bo false } } + +fn check_the_operation_proposal_error(error: &CmdError) -> bool { + let text = "script failed: revert: Operation with this proposal id already exists"; + if let Some(stderr) = &error.stderr { + stderr.contains(text) + } else { + false + } +} diff --git a/zk_toolbox/crates/zk_inception/src/accept_ownership.rs b/zk_toolbox/crates/zk_inception/src/accept_ownership.rs index 179cb696ac3d..7217a629cdd7 100644 --- a/zk_toolbox/crates/zk_inception/src/accept_ownership.rs +++ b/zk_toolbox/crates/zk_inception/src/accept_ownership.rs @@ -2,14 +2,13 @@ use common::{ forge::{Forge, ForgeScript, ForgeScriptArgs}, spinner::Spinner, }; -use config::{ - forge_interface::{ - accept_ownership::AcceptOwnershipInput, script_params::ACCEPT_GOVERNANCE_SCRIPT_PARAMS, - }, - traits::SaveConfig, - EcosystemConfig, +use config::{forge_interface::script_params::ACCEPT_GOVERNANCE_SCRIPT_PARAMS, EcosystemConfig}; +use ethers::{ + abi::parse_abi, + contract::BaseContract, + types::{Address, H256}, }; -use ethers::types::{Address, H256}; +use lazy_static::lazy_static; use xshell::Shell; use crate::{ @@ -17,6 +16,16 @@ use crate::{ utils::forge::{check_the_balance, fill_forge_private_key}, }; +lazy_static! { + static ref ACCEPT_ADMIN: BaseContract = BaseContract::from( + parse_abi(&[ + "function acceptOwner(address governor, address target) public", + "function acceptAdmin(address governor, address target) public" + ]) + .unwrap(), + ); +} + pub async fn accept_admin( shell: &Shell, ecosystem_config: &EcosystemConfig, @@ -26,6 +35,12 @@ pub async fn accept_admin( forge_args: &ForgeScriptArgs, l1_rpc_url: String, ) -> anyhow::Result<()> { + let mut forge_args = forge_args.clone(); + forge_args.resume = false; + + let calldata = ACCEPT_ADMIN + .encode("acceptAdmin", (governor_contract, target_address)) + .unwrap(); let foundry_contracts_path = ecosystem_config.path_to_foundry(); let forge = Forge::new(&foundry_contracts_path) .script( @@ -35,16 +50,8 @@ pub async fn accept_admin( .with_ffi() .with_rpc_url(l1_rpc_url) .with_broadcast() - .with_signature("acceptAdmin()"); - accept_ownership( - shell, - ecosystem_config, - governor_contract, - governor, - target_address, - forge, - ) - .await + .with_calldata(&calldata); + accept_ownership(shell, governor, forge).await } pub async fn accept_owner( @@ -56,6 +63,13 @@ pub async fn accept_owner( forge_args: &ForgeScriptArgs, l1_rpc_url: String, ) -> anyhow::Result<()> { + // resume doesn't properly work here. + let mut forge_args = forge_args.clone(); + forge_args.resume = false; + + let calldata = ACCEPT_ADMIN + .encode("acceptOwner", (governor_contract, target_address)) + .unwrap(); let foundry_contracts_path = ecosystem_config.path_to_foundry(); let forge = Forge::new(&foundry_contracts_path) .script( @@ -65,37 +79,16 @@ pub async fn accept_owner( .with_ffi() .with_rpc_url(l1_rpc_url) .with_broadcast() - .with_signature("acceptOwner()"); - accept_ownership( - shell, - ecosystem_config, - governor_contract, - governor, - target_address, - forge, - ) - .await + .with_calldata(&calldata); + accept_ownership(shell, governor, forge).await } async fn accept_ownership( shell: &Shell, - ecosystem_config: &EcosystemConfig, - governor_contract: Address, governor: Option, - target_address: Address, mut forge: ForgeScript, ) -> anyhow::Result<()> { - let input = AcceptOwnershipInput { - target_addr: target_address, - governor: governor_contract, - }; - input.save( - shell, - ACCEPT_GOVERNANCE_SCRIPT_PARAMS.input(&ecosystem_config.link_to_code), - )?; - forge = fill_forge_private_key(forge, governor)?; - check_the_balance(&forge).await?; let spinner = Spinner::new(MSG_ACCEPTING_GOVERNANCE_SPINNER); forge.run(shell)?; From bb48cff91c3e7c6c37eefd77f5ec8064252b81a0 Mon Sep 17 00:00:00 2001 From: Danil Date: Tue, 2 Jul 2024 19:15:13 +0200 Subject: [PATCH 3/5] Suppress errors from foundry Signed-off-by: Danil --- contracts | 2 +- zk_toolbox/crates/common/src/forge.rs | 72 ++++++++++--------- .../zk_inception/src/accept_ownership.rs | 3 + 3 files changed, 41 insertions(+), 36 deletions(-) diff --git a/contracts b/contracts index 8172969672cc..eb1df887e733 160000 --- a/contracts +++ b/contracts @@ -1 +1 @@ -Subproject commit 8172969672cc6a38542cd8f5578c74b7e30cd3b4 +Subproject commit eb1df887e7338d46bbe91b54113d15bb54552af5 diff --git a/zk_toolbox/crates/common/src/forge.rs b/zk_toolbox/crates/common/src/forge.rs index c46a1e7c95ff..cf2c9bb1523a 100644 --- a/zk_toolbox/crates/common/src/forge.rs +++ b/zk_toolbox/crates/common/src/forge.rs @@ -16,7 +16,7 @@ use strum_macros::Display; use xshell::{cmd, Shell}; use crate::{ - cmd::{Cmd, CmdError}, + cmd::{Cmd, CmdResult}, ethereum::create_ethers_client, }; @@ -59,33 +59,17 @@ impl ForgeScript { let _dir_guard = shell.push_dir(&self.base_path); let script_path = self.script_path.as_os_str(); let args_no_resume = self.args.build(); + let command = format!("forge script {script_path:?} --legacy"); if self.args.resume { - // Resume doesn't work if the forge script has never been started on this chain before. - // So we want to catch it and try again without resume arg if it's the case let mut args = args_no_resume.clone(); args.push(ForgeScriptArg::Resume.to_string()); - if let Err(err) = - Cmd::new(cmd!(shell, "forge script {script_path} --legacy {args...}")).run() - { - if !check_for_resume_not_successful_because_has_not_began(&err) { - return Err(err.into()); - } - } else { - return Ok(()); - }; + return Ok(Cmd::new(cmd!(shell, "{command} {args...}")) + .run() + .suppress_resume_not_successful_because_has_not_began()?); } - if let Err(err) = Cmd::new(cmd!( - shell, - "forge script {script_path} --legacy {args_no_resume...}" - )) - .run() - { - if check_the_operation_proposal_error(&err) { - return Ok(()); - } - return Err(err.into()); - } - Ok(()) + Ok(Cmd::new(cmd!(shell, "{command} {args_no_resume...}")) + .run() + .suppress_proposal_error()?) } pub fn wallet_args_passed(&self) -> bool { @@ -389,19 +373,37 @@ pub enum ForgeVerifier { Oklink, } -fn check_for_resume_not_successful_because_has_not_began(error: &CmdError) -> bool { - if let Some(stderr) = &error.stderr { - stderr.contains("Deployment not found for chain") - } else { - false +// Trait for handling forge errors. Required for implementing method for CmdResult +trait ForgeErrorHandler { + // Resume doesn't work if the forge script has never been started on this chain before. + // So we want to catch it and try again without resume arg if it's the case + fn suppress_resume_not_successful_because_has_not_began(self) -> Self; + // Catch the error if upgrade tx has already been processed. We do execute much of + // txs using upgrade mechanism and if this particular upgrade has already been processed we could assume + // it as a success + fn suppress_proposal_error(self) -> Self; +} + +impl ForgeErrorHandler for CmdResult<()> { + fn suppress_resume_not_successful_because_has_not_began(self) -> Self { + let text = "Deployment not found for chain"; + suppress_error(self, text) + } + + fn suppress_proposal_error(self) -> Self { + let text = "revert: Operation with this proposal id already exists"; + suppress_error(self, text) } } -fn check_the_operation_proposal_error(error: &CmdError) -> bool { - let text = "script failed: revert: Operation with this proposal id already exists"; - if let Some(stderr) = &error.stderr { - stderr.contains(text) - } else { - false +fn suppress_error(cmd_result: CmdResult<()>, error_text: &str) -> CmdResult<()> { + if let Err(cmd_error) = &cmd_result { + if let Some(stderr) = &cmd_error.stderr { + if stderr.contains(error_text) { + return Ok(()); + } + } + return cmd_result; } + return cmd_result; } diff --git a/zk_toolbox/crates/zk_inception/src/accept_ownership.rs b/zk_toolbox/crates/zk_inception/src/accept_ownership.rs index 7217a629cdd7..a236d437af53 100644 --- a/zk_toolbox/crates/zk_inception/src/accept_ownership.rs +++ b/zk_toolbox/crates/zk_inception/src/accept_ownership.rs @@ -35,6 +35,9 @@ pub async fn accept_admin( forge_args: &ForgeScriptArgs, l1_rpc_url: String, ) -> anyhow::Result<()> { + // Resume for accept admin doesn't work properly. Foundry assumes that if signature of the function is the same, + // than it's the same call, but because we are calling this function multiple times during the init process, + // code assumes that doing only once is enough, but actually we need to accept admin multiple times let mut forge_args = forge_args.clone(); forge_args.resume = false; From 60709ce99b5d7eb1c6445d5e790e6787aa0a3930 Mon Sep 17 00:00:00 2001 From: Danil Date: Wed, 3 Jul 2024 11:30:43 +0200 Subject: [PATCH 4/5] Fix cmd usage Signed-off-by: Danil --- zk_toolbox/crates/common/src/forge.rs | 41 ++++++++++++++------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/zk_toolbox/crates/common/src/forge.rs b/zk_toolbox/crates/common/src/forge.rs index cf2c9bb1523a..de91c0e72500 100644 --- a/zk_toolbox/crates/common/src/forge.rs +++ b/zk_toolbox/crates/common/src/forge.rs @@ -59,17 +59,23 @@ impl ForgeScript { let _dir_guard = shell.push_dir(&self.base_path); let script_path = self.script_path.as_os_str(); let args_no_resume = self.args.build(); - let command = format!("forge script {script_path:?} --legacy"); if self.args.resume { let mut args = args_no_resume.clone(); args.push(ForgeScriptArg::Resume.to_string()); - return Ok(Cmd::new(cmd!(shell, "{command} {args...}")) - .run() - .suppress_resume_not_successful_because_has_not_began()?); + let res = Cmd::new(cmd!(shell, "forge script {script_path} --legacy {args...}")).run(); + if !res.resume_not_successful_because_has_not_began() { + return Ok(res?); + } + } + let res = Cmd::new(cmd!( + shell, + "forge script {script_path} --legacy {args_no_resume...}" + )) + .run(); + if res.proposal_error() { + return Ok(()); } - Ok(Cmd::new(cmd!(shell, "{command} {args_no_resume...}")) - .run() - .suppress_proposal_error()?) + Ok(res?) } pub fn wallet_args_passed(&self) -> bool { @@ -377,33 +383,30 @@ pub enum ForgeVerifier { trait ForgeErrorHandler { // Resume doesn't work if the forge script has never been started on this chain before. // So we want to catch it and try again without resume arg if it's the case - fn suppress_resume_not_successful_because_has_not_began(self) -> Self; + fn resume_not_successful_because_has_not_began(&self) -> bool; // Catch the error if upgrade tx has already been processed. We do execute much of // txs using upgrade mechanism and if this particular upgrade has already been processed we could assume // it as a success - fn suppress_proposal_error(self) -> Self; + fn proposal_error(&self) -> bool; } impl ForgeErrorHandler for CmdResult<()> { - fn suppress_resume_not_successful_because_has_not_began(self) -> Self { + fn resume_not_successful_because_has_not_began(&self) -> bool { let text = "Deployment not found for chain"; - suppress_error(self, text) + check_error(self, text) } - fn suppress_proposal_error(self) -> Self { + fn proposal_error(&self) -> bool { let text = "revert: Operation with this proposal id already exists"; - suppress_error(self, text) + check_error(self, text) } } -fn suppress_error(cmd_result: CmdResult<()>, error_text: &str) -> CmdResult<()> { +fn check_error(cmd_result: &CmdResult<()>, error_text: &str) -> bool { if let Err(cmd_error) = &cmd_result { if let Some(stderr) = &cmd_error.stderr { - if stderr.contains(error_text) { - return Ok(()); - } + return stderr.contains(error_text); } - return cmd_result; } - return cmd_result; + false } From 0b7f6dd73b42dc3d0a599ecdeb90e7e15d935982 Mon Sep 17 00:00:00 2001 From: Danil Date: Thu, 4 Jul 2024 18:17:43 +0200 Subject: [PATCH 5/5] Update contracts Signed-off-by: Danil --- contracts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts b/contracts index eb1df887e733..f4ae6a1b90e2 160000 --- a/contracts +++ b/contracts @@ -1 +1 @@ -Subproject commit eb1df887e7338d46bbe91b54113d15bb54552af5 +Subproject commit f4ae6a1b90e2c269542848ada44de669a5009290