-
Notifications
You must be signed in to change notification settings - Fork 125
feat(l2): accelerate deployer #5394
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
base: main
Are you sure you want to change the base?
Conversation
Lines of code reportTotal lines added: Detailed view |
| let deploy_tx_hash = | ||
| send_tx_bump_gas_exponential_backoff(eth_client, deploy_tx, deployer).await?; | ||
|
|
||
| wait_for_transaction_receipt(deploy_tx_hash, eth_client, 10).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
send_tx_bump_gas_exponential_backoff already waits for the receipt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR accelerates the L2 contract deployer by sending all transactions asynchronously and waiting for receipts at the end, rather than waiting for each transaction individually.
Key Changes:
- Created non-waiting versions of deployment and initialization functions (
*_no_wait) in the SDK - Modified the deployer to send all deployment transactions first, then all initialization transactions, and finally wait for all receipts in batch
- Added explicit nonce management throughout to ensure correct transaction ordering when not waiting for receipts
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| crates/l2/sdk/src/sdk.rs | Added non-waiting versions of deployment functions (create2_deploy_no_wait, deploy_proxy_no_wait, deploy_with_proxy_from_bytecode_no_wait, etc.) and refactored existing functions to support asynchronous operations with manual nonce management |
| cmd/ethrex/l2/deployer.rs | Updated deployment and initialization logic to use non-waiting functions, added explicit nonce tracking, collected transaction hashes, and implemented batch receipt waiting at the end of the process |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/ethrex/l2/deployer.rs
Outdated
| } | ||
| let receipt = wait_for_transaction_receipt(tx_hash, ð_client, 100).await?; | ||
| if !receipt.receipt.status { | ||
| error!("Receipt status is false for tx_hash: {:#x}", tx_hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
| error!("Receipt status is false for tx_hash: {:#x}", tx_hash); | |
| error!("Receipt status is false for tx_hash: {tx_hash:#x}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 2146f6e!
cmd/ethrex/l2/deployer.rs
Outdated
| info!( | ||
| "Gas used for tx_hash {:#x}: {}", | ||
| tx_hash, receipt.tx_info.gas_used | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| info!( | |
| "Gas used for tx_hash {:#x}: {}", | |
| tx_hash, receipt.tx_info.gas_used | |
| ); | |
| info!( | |
| "Gas used for tx_hash {tx_hash:#x}: {}", | |
| receipt.tx_info.gas_used | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log was for debugging. Removed 2146f6e!
cmd/ethrex/l2/deployer.rs
Outdated
| } | ||
| let receipt = wait_for_transaction_receipt(tx_hash, ð_client, 100).await?; | ||
| if !receipt.receipt.status { | ||
| error!("Receipt status is false for tx_hash: {:#x}", tx_hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| error!("Receipt status is false for tx_hash: {:#x}", tx_hash); | |
| error!("Receipt status is false for tx_hash: {tx_hash:#x}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
cmd/ethrex/l2/deployer.rs
Outdated
|
|
||
| info!("Waiting for initialization transactions receipts"); | ||
|
|
||
| for tx_hash in initialize_tx_hashes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do something like depoy_tx_hashes.extend(initialize_tx_hashes) and iterate that to avoid repeating code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 2146f6e!
| ..Default::default() | ||
| }, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd increment the nonce here even tough we don't use it anymore, to avoid future problems if we add some new transaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler complains with:
value assigned to `nonce` is never read
cmd/ethrex/l2/deployer.rs
Outdated
| info!( | ||
| transfer_tx_hash = %format!("{transfer_tx_hash:#x}"), | ||
| accept_tx_hash = %format!("{accept_tx_hash:#x}"), | ||
| "CommonBridge ownership transferred and accepted" | ||
| ); | ||
| info!("Used nonce {}", nonce); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| info!( | |
| transfer_tx_hash = %format!("{transfer_tx_hash:#x}"), | |
| accept_tx_hash = %format!("{accept_tx_hash:#x}"), | |
| "CommonBridge ownership transferred and accepted" | |
| ); | |
| info!("Used nonce {}", nonce); | |
| info!( | |
| transfer_tx_hash = %format!("{transfer_tx_hash:#x}"), | |
| accept_tx_hash = %format!("{accept_tx_hash:#x}"), | |
| nonce, | |
| "CommonBridge ownership transferred and accepted" | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log was for debugging. Removed!
| ..Default::default() | ||
| }, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a nonce increment is missing here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 2146f6e!
| let accept_tx_hash = send_generic_transaction(eth_client, accept_tx, &signer).await?; | ||
| wait_for_transaction_receipt(accept_tx_hash, eth_client, 100).await?; | ||
|
|
||
| tx_hashes.push(accept_tx_hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here about incrementing the nonce even it's not needed
| } | ||
|
|
||
| /// Same as `deploy_with_proxy`, but does not wait for the transaction receipts. | ||
| pub async fn deploy_with_proxy_no_wait( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the same as deploy_with_proxy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Refactored here 2146f6e!
| /// without the `COMPILE_CONTRACTS` env var set. In that case, this function | ||
| /// will return `DeployError::ProxyBytecodeNotFound`. | ||
| async fn deploy_proxy( | ||
| pub async fn create2_deploy_from_bytecode_no_wait( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make the regular version call the _no_wait version and the wait for the receipt. I think that would be less repeatitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 2146f6e!
Motivation
Our deployer currently waits for all transaction receipts before moving on to the next transaction.
Description
Send all transactions asynchronously and wait for all receipts at the end.
Closes None