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

fix: Zksync gas estimation fix for aggregation ISMs #5220

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: Zksync gas estimation fix for aggregation ISMs #5220

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b5b5bec
initial attempt fix
anson-caldera Jan 20, 2025
00830f7
cargo lock?
anson-caldera Jan 20, 2025
7a691bc
merge?
anson-caldera Jan 20, 2025
3df07e7
fix merge
anson-caldera Jan 20, 2025
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
12 changes: 8 additions & 4 deletions rust/main/chains/hyperlane-ethereum/src/ism/interchain_security_module.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::sync::Arc;

use async_trait::async_trait;
use ethers::providers::Middleware;
use ethers_core::abi::ethereum_types::H160;
use tracing::{instrument, warn};

use futures_util::future::try_join;
Expand Down Expand Up @@ -116,10 +117,13 @@ where
message: &HyperlaneMessage,
metadata: &[u8],
) -> ChainResult<Option<U256>> {
let tx = self.contract.verify(
metadata.to_owned().into(),
RawHyperlaneMessage::from(message).to_vec().into(),
);
let tx = self
.contract
.verify(
metadata.to_owned().into(),
RawHyperlaneMessage::from(message).to_vec().into(),
)
.from(H160::random()); // We generate a random from address to ensure compatibility with zksync
Copy link
Contributor

Choose a reason for hiding this comment

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

appreciate the PR and investigation! 🙏

i had a look at zkSync-Community-Hub/zksync-developers#144 (reply in thread) and noticed it says that even when the from field is specified, estimation can still fail because from is not pointing to an actual smart account. This was the case with the Safe address in that thread:

When processing a transaction, the protocol checks the code of the from account and, in this case, as Safe accounts are not deployed as native accounts on zkSync (via createAccount or create2Account), it fails with the error above

so i wonder if it makes more sense to set from to the relayer's smart account address set in the provider, to avoid running into a similar error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is because of Safe-specific behavior. In the comment they mentioned, Safe accounts are not deployed as native accounts on zkSync (via createAccount or create2Account). This is likely because they're contracts that weren't deployed as accounts. What they're saying in that thread is that it the from address has to be an account.

As I've successfully processed a transaction with this, it stands that the random addresses are interpreted as accounts despite not being initialized. This shouldn't encounter any errors, at least not above the probability of collision. It was probably failing for the Safe addresses because they were initialized as non-accounts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, can you please add this context to the comment

Copy link
Contributor

Choose a reason for hiding this comment

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

actually i'm fairly certain estimation fails on certain chains if from has insufficient funds to theoretically pay for the simulated tx. We'd need to pass the signer address configured in the provider to make sure we don't run into that. Would you be down to do make that change as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then we'd run into the problem where if the relayer is a Safe on zksync it stops working, no? I've tested this on op bedrock, arb nitro, and zksync rollups, and it works on all three. Can you give an example chain where this might fail so I can test that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.ethers.org/v5/troubleshooting/errors/#help-INSUFFICIENT_FUNDS

I think that only applies when a gas price is specified, which isn't the case in this call. Happy to test this on the chains

let (verifies, gas_estimate) = try_join(tx.call(), tx.estimate_gas()).await?;
if verifies {
Ok(Some(gas_estimate.into()))
Expand Down
Loading