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.

Sign up for GitHub

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

Jump to bottom

Multichain Scripts: Deploy each sequence of transactions sequentially instead of in parallel. #6271

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Multichain Scripts: Deploy each sequence of transactions sequentially instead of in parallel. #6271

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
183eec2
deploy recorded txns sequentially
ArshanKhanifar Nov 9, 2023
787001d
remove println
ArshanKhanifar Nov 9, 2023
2e30d70
fmt fix
ArshanKhanifar Nov 9, 2023
b0dc520
remove unnecessary comment
ArshanKhanifar Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 22 additions & 24 deletions crates/forge/bin/cmd/script/multi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use super::{
ScriptArgs,
};
use ethers::signers::LocalWallet;
use eyre::{ContextCompat, Result, WrapErr};
use eyre::{ContextCompat, Report, Result, WrapErr};
use foundry_cli::utils::now;
use foundry_common::{fs, get_http_provider};
use foundry_compilers::{artifacts::Libraries, ArtifactId};
Expand Down Expand Up @@ -139,37 +139,35 @@ impl ScriptArgs {
join_all(futs).await.into_iter().filter(|res| res.is_err()).collect::<Vec<_>>();

if !errors.is_empty() {
return Err(eyre::eyre!("{errors:?}"))
return Err(eyre::eyre!("{errors:?}"));
}
}

trace!(target: "script", "broadcasting multi chain deployments");

let futs = deployments
.deployments
.iter_mut()
.map(|sequence| async {
match self
.send_transactions(
sequence,
&sequence.typed_transactions().first().unwrap().0.clone(),
&script_wallets,
)
.await
{
Ok(_) => {
if self.verify {
return sequence.verify_contracts(config, verify.clone()).await
}
Ok(())
let mut results: Vec<Result<(), Report>> = Vec::new(); // Declare a Vec of Result<(), Report>
Copy link
Member

Choose a reason for hiding this comment

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

no need for this comment, as the vec is already typed


for sequence in deployments.deployments.iter_mut() {
let result = match self
.send_transactions(
sequence,
&sequence.typed_transactions().first().unwrap().0.clone(),
&script_wallets,
)
.await
{
Ok(_) => {
if self.verify {
return sequence.verify_contracts(config, verify.clone()).await
}
Err(err) => Err(err),
Ok(())
}
})
.collect::<Vec<_>>();
Err(err) => Err(err),
};
results.push(result);
}

let errors =
join_all(futs).await.into_iter().filter(|res| res.is_err()).collect::<Vec<_>>();
let errors = results.into_iter().filter(|res| res.is_err()).collect::<Vec<_>>();
Copy link
Member

@Evalir Evalir Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right, so I see what's going on:

  • Before, we drove all futures to completion depending on availability (ideally in order), but this wasn't guaranteed—what's guaranteed is that they're stored in the order they were provided.
  • Now, we're waiting for each batch to complete and storing its result, avoiding the race condition

This might mean that complex multichain deployments might take longer though, but i'm willing to take the hit for less failures

Copy link
Contributor Author

@ArshanKhanifar ArshanKhanifar Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, but the potential temporal dependencies between the batches would result in script failures.

I could still see people running into issues if their scripts use blockchain state that changes too quickly even after one block passing: i.e. getting the price of ETH in a ETH/USDC uni pool, and using that as an input to another script.

But for the most part I feel like people just wanna deploy their contracts & maybe do some configurations on them. Not so much use specific chain-state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and yeah, complex multichain deployments would take longer. I feel like it's a worthwhile tradeoff for the amount of flexibility that's enabled by it.


if !errors.is_empty() {
return Err(eyre::eyre!("{errors:?}"))
Expand Down