-
Notifications
You must be signed in to change notification settings - Fork 76
fix: 0.14.0 E2E tests and change settlement to handle fusaka upgrade #841
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
Conversation
…net and sepolia ethereum * chore: refactor the code * fix tests * fix clippy issues
| for proof in proofs.iter() { | ||
| blob_cell_proofs.push(FixedBytes::new(proof.to_bytes().into_inner())); | ||
| } |
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.
instead of iter, you can probably use something like extend
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.
sorted
| /// Method to create a cell transaction (post-Fusaka, for sepolia/testnet) | ||
| /// Creates transaction with cell proofs | ||
| /// Returns the signed transaction variant with EIP7594 sidecar | ||
| async fn create_cell_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.
if the only difference is a sidecar, we don't need to copy paste the entire 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.
sorted
heemankv
left a comment
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.
Looks good,
I think we can make the code more idiomatic tho,
specially in eth settlement's conversion.rs and lib.rs
| match nonce { | ||
| Some(nonce) => self.execute_v3(calls).nonce(nonce.into()), | ||
| None => self.execute_v3(calls), | ||
| None => self.execute_v3(calls).l1_gas(0).l2_gas(0).l1_data_gas(0), |
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.
Comment on why this is needed would be helpful !
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
e2e/src/services/madara/config.rs
Outdated
| l1_endpoint: None, | ||
| l1_gas_price: 0, | ||
| blob_gas_price: 0, | ||
| l1_gas_price: 1, |
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.
Comment explaining this would be helpful
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
| pub type DefaultHttpProvider = alloy::providers::fillers::FillProvider< | ||
| alloy::providers::fillers::JoinFill< | ||
| alloy::providers::Identity, | ||
| alloy::providers::fillers::JoinFill< | ||
| alloy::providers::fillers::GasFiller, | ||
| alloy::providers::fillers::JoinFill< | ||
| alloy::providers::fillers::BlobGasFiller, | ||
| alloy::providers::fillers::JoinFill< | ||
| alloy::providers::fillers::NonceFiller, | ||
| alloy::providers::fillers::ChainIdFiller, | ||
| >, | ||
| >, | ||
| >, | ||
| >, | ||
| alloy::providers::RootProvider<alloy::network::Ethereum>, | ||
| alloy::network::Ethereum, | ||
| >; |
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.
The same type is being used in prover-clients as well, can we create it in utils and use it from there ?
Also comment in why this is the way it is now would be great!
|
|
||
| /// Whether settling on Ethereum mainnet (true) or Sepolia testnet (false). | ||
| /// Mainnet uses blob proofs (pre-Fusaka), Sepolia uses cell proofs (post-Fusaka). | ||
| #[arg(env = "MADARA_ORCHESTRATOR_ETHEREUM_IS_MAINNET", long, default_value = "false")] |
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 is only till December 3 2025 (Fusaka launch date on Mainnet) right ?
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.
Updated the flag to make it more specific to the usecase as discusses with @apoorvsadana. Not it's called disable-peerdas.
Yes, it's till mainnet also have fusaka. After that we can forget about this flag
…ecar * refactor is_mainnet flag to disable_peerdas
…to fix/0.14.0/e2e
apoorvsadana
left a comment
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.
prepare_sidecar functions looks good now!
54faaec to
082472a
Compare
|
btw, do we also need to enable it back in the CI in this PR? |
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
What is the current behavior?
E2E fails. Settlement fails on Ethereum sepolia
Resolves: #NA
What is the new behavior?
This PR fixes the issue in the E2E test because of 0.14.0 update in Madara. It also updates the state update job (and ethereum settlement client) to handle the fusaka upgrade on sepolia.
Does this introduce a breaking change?
Other information