fix(ethexe/ethereum): use gas limit from estimate_gas, add limit to max_fee_per_gas_in_gwei#5375
fix(ethexe/ethereum): use gas limit from estimate_gas, add limit to max_fee_per_gas_in_gwei#5375StackOverflowExcept1on wants to merge 6 commits intomasterfrom
Conversation
Changed Files
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the gas limit estimation process for batch commitments. By replacing the previous max-based fallback with a direct estimation and an explicit validation step, the system now ensures that gas limits remain within acceptable bounds while providing better error reporting when limits are exceeded. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the gas limit calculation in the Ethereum router to explicitly check against HUGE_GAS_LIMIT and return an error if exceeded, replacing the previous logic that used a max operation. A review comment suggests simplifying the error message in the anyhow! macro to remove redundancy with the existing log entry.
|
@claude opus review |
|
Claude finished @StackOverflowExcept1on's task in 2m 31s —— View job Claude PR ReviewSummary: This PR replaces the previous Overall the direction is solid — tightening gas use and adding a fee ceiling for validator cost control is the right call. Comments below, ordered by severity. severity: medium — loss of safety margin on submitted gas limit File: Previously The 30k buffer covers Worth quantifying via a run on Sepolia with tight gas — if we observe no OOG reverts over N batches, the margin is fine; otherwise this will show up as flaky validator tx failures. severity: medium — extra RPC round-trip on every batch commitment File:
Options: (a) skip the call entirely when if self.eip1559_max_fee_per_gas_in_gwei > 0 {
let Eip1559Estimation { max_fee_per_gas, .. } = self
.instance
.provider()
.estimate_eip1559_fees_with(self.eip1559_estimator.clone())
.await?;
// ... check ...
}severity: low — misleading default in example configs Files: # EIP-1559 max fee per gas in gwei for transaction fee estimation (for batch commitments).
# (optional, default: 20).
# eip1559-max-fee-per-gas-in-gwei = 20The actual default resolved in code is Contrast with severity: low — stray closing paren in error message File: return Err(anyhow!(
"Estimated max fee per gas {max_fee_per_gas_in_gwei} gwei is higher than the configured maximum of {eip1559_max_fee_per_gas_in_gwei} gwei, refusing to commit batch)",
));Trailing severity: low — gwei truncation in the ceiling check File: let max_fee_per_gas_in_gwei = max_fee_per_gas / (GWEI_TO_WEI as u128);Integer division floors, so a fee of e.g. 25.9 gwei becomes 25. If the user sets the cap to 25 gwei, a 25.9 gwei mempool price will slip through. Not a security issue (the intent is "roughly cap cost"), but worth either noting in the comment or switching to severity: low — zero-as-disabled sentinel File:
severity: low — unrelated workflow change in the same PR Files: Adds an Nit (agreed with gemini-code-assist): the Todo
|
|
|
||
| # EIP-1559 max fee per gas in gwei for transaction fee estimation (for batch commitments). | ||
| # (optional, default: 0). | ||
| # eip1559-max-fee-per-gas-in-gwei = 0 |
There was a problem hiding this comment.
The max_fee_per_gas = 0 means, that TX will never be included, because at least base_fee always > 0.
Did you mean max_priority_fee_per_gas = 0?
There was a problem hiding this comment.
0 means limit is disabled by default
No description provided.