Fix estiamte gas l1 fee issue when metamask max amount is selected #1261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
auto-assign
bot
requested review from
eyusufatik,
jfldde,
kpp,
rakanalh and
yaziciahmet
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
|
|
I do understand the motivation, but I have some concerns about the solution. With this, if someone wants to actually send 0 eth, they will be charged extra even though there are no state changes. I am aware that sending 0 eth is not a common operation, and also the fee difference is going to be little, but still wanted to raise this to understand further. Do we know what does Reth to resolve this? |
no one is charged extra, they just get a higher estimate for gas gas fee charged will be the exact usage. reth does not have a problem with this, we are trying to get wallet's / devs calculate L1 fee without ever knowing about it :D |
yaziciahmet
approved these changes
Oct 2, 2024
eyusufatik
approved these changes
Oct 2, 2024
crates/evm/src/query.rs
Outdated
| @@ -71,8 +73,7 @@ impl EstimatedTxExpenses { | |||
| /// Return total estimated gas used including evm gas and L1 fee. | |||
| pub(crate) fn gas_with_l1_overhead(&self) -> U256 { | |||
| // Actually not an L1 fee but l1_fee / base_fee. | |||
| let l1_fee_overhead = | |||
| U256::from(1).max(self.l1_fee / (self.base_fee + U256::from(GWEI_TO_WEI))); // assume 1 gwei priority fee | |||
| let l1_fee_overhead = U256::from(1).max(self.l1_fee.div_ceil(self.base_fee)); // assume 1 gwei priority fee | |||
eyusufatik
approved these changes
Oct 2, 2024
eyusufatik
added a commit
that referenced
this pull request
Oct 9, 2024
* Structured concurrency: Part 1 (#1214) * Implement managed tasks for SubscriptionManager * Fix clippy * Add comment about trace task * Use broadcast instead of mpsc for l2 blocks events * Fix typo (#1246) * Structured Concurrency: Part 2 (#1216) * Implement managed tasks for SubscriptionManager * Fix clippy * Add comment about trace task * Use broadcast instead of mpsc for l2 blocks events * Fix spawns in DaService * Add TaskManager * Use task manager in sequencer * Document TaskManager * Handle shutdown event * Use TaskTracker * Add comment about using a cancellation token * Use JoinHandles instead of TaskTracker * Use TaskManager in fullnode and prover * Improve bitcoin-da service * Force spawned tasks to accept a cancellation token * Use biased polling * Satisfy clippy * Address PR feedback * Fix checks * Pin foundry (#1253) * Pin foundary * Add comment * Remove puclihs mock da block script * Use default block time on error (#1244) * Use default block time on error * Constant target_block_time * Remove import * Dump logs and cleanup on assert failures (#1252) * Fix estiamte gas l1 fee issue when metamask max amount is selected (#1261) * Fix estiamte gas l1 fee issue when metamask max amount is selected * Fix tests * Remove unnecessary comment * Fix hive (#1263) * Fix configs * Remove publish mock block from docker * Update port for hive * Rename config in hive * Refactor eth estimate gas l1 fee issue (#1264) * Refactor eth estimate gas l1 fee issue * Nits * Fix bug * Refactor * Do not ignore resources * Fix --------- Co-authored-by: Roman Proskuryakoff <[email protected]> * Pruning skeleton (#1229) * Implement managed tasks for SubscriptionManager * Fix clippy * Add comment about trace task * Use broadcast instead of mpsc for l2 blocks events * Fix spawns in DaService * Add TaskManager * Use task manager in sequencer * Document TaskManager * Handle shutdown event * Use TaskTracker * Add comment about using a cancellation token * Use JoinHandles instead of TaskTracker * Use TaskManager in fullnode and prover * Improve bitcoin-da service * Force spawned tasks to accept a cancellation token * Use biased polling * Satisfy clippy * WIP * Add pruning tables * Pruning skeleton implementation * Use pruner in nodes * Use biased polling based on order * WIP * Fix how config is done * Derive default * Add logs * Let the tasks finish without panicing * Use pruning config in fullnode and prover * Add simple run test * Use option instead of PruningMode * Unneccessary changes * l2_receiver * Cleanup prints * Use last pruned block in calculation * Implement pruning criteria * Lint and add comment * Set the last_pruned_block to up_to_block value * Don't store config internally * Remove from constructor * Should not change * Move config value * Remove pruning from sequencer / prover * Derive SequencerClient (#1269) * Derive SequencerClient * Renames * Lint * Improve estimate gas and create access list rpcs (#1265) * Remove unnecessary transact_to conversion * unwrap_or_default * More concise block_env initialization * Actually get highest gas limit from request and block env * One liner set * Allow unused * Cleanup prepare_call_env * Fix tests & lint * Replace unwrap * Set state to block in eth_call * Consume request in prepare_call_env * Lint * Set state before reading config * Remove unnecessary clone * Replace allow unused with feature native gate * Add sys txs to evm tests (#1255) * modify config_push_contracts * modify call_multiple_test * modify tests in call_tests.rs * moving common functions to utils.rs * minor fixes * updated lock files * modify tests --------- Co-authored-by: Esad Yusuf Atik <[email protected]> * make prover config arg not optional (#1278) * Pin to 27.1 (#1279) * E2E tests using citrea-e2e (#1277) * E2E tests using citrea-e2e * Lint * Set CITREA path * With github.workspace * Target citrea-e2e main * Update citrea-e2e rev * Use debug build * Update citrea-e2e * Prover generate input rpc (#1280) * WIP Implement prover generate rpcg * It compiles but stf is modified * Context * Fix Context: Send * Merge fix * Move function from common to prover * Remove code duplicate and unnecessary log * Add optional parameter to break commitments into groups * Return input as string --------- Co-authored-by: Roman Proskuryakoff <[email protected]> * Move node configs to citrea-common (#1286) * move node configs to a seperate crate * fix udeps * remove native feature * move SequencerConfig * move config to citrea-common * fix use statements * Update e2e test framework and fix tests (#1305) * Update e2e test framework and fix tests * Fix bug * Update ci binary env key * Test if new fix works * Update bitcoincore-rpc version * Dprint * Try against fix prover config rev * Target main HEAD rev --------- Co-authored-by: jfldde <[email protected]> * build and publish a new image for every commit to the nightly branch (#1309) * Get fee recommendation from mempool space (#1302) * Get fee recommendation from mempool space * Fix bug * Construct mempool space endpoint by network --------- Co-authored-by: Esad Yusuf Atik <[email protected]> * new path fix for nightly (#1310) * Enable pending block tag in simulation endpoints (#1303) * treat pending tag same as latest * fix lint * return new sealed block for pending * handle pending tag externally * revert enabling pending in some endpoints * get blockenv instead of sealed block * address review comments * implement tests for eth_call, eth_estimateGas, eth_createAccessList pending blocks * rename tests --------- Co-authored-by: eyusufatik <[email protected]> * Implement state and block overrides (#1270) * Implement state and block overrides * Update comment * apply_block_overrides func * apply_state_overrides func * Use a single func for replacing account storage * Clippy * Address feedback Use Reth's BlockOverrides with saturating_to to convert back to u64 * Test for block overrides * Test for state overrides * Comment tests * Create a fresh working set * Remove box * remove alloy-serde --------- Co-authored-by: eyusufatik <[email protected]> * Use spawn_blocking for da queue (#1311) * Fetch smart fee only if none (#1312) * update run doc and changelog (#1315) --------- Co-authored-by: Rakan Al-Huneiti <[email protected]> Co-authored-by: Ahmet Yazıcı <[email protected]> Co-authored-by: Erce Can Bektüre <[email protected]> Co-authored-by: jfldde <[email protected]> Co-authored-by: Erce Can Bektüre <[email protected]> Co-authored-by: Roman Proskuryakoff <[email protected]> Co-authored-by: Ege Okan Ünaldı <[email protected]> Co-authored-by: Çetin <[email protected]>
eyusufatik
added a commit
that referenced
this pull request
Oct 9, 2024
* Structured concurrency: Part 1 (#1214) * Implement managed tasks for SubscriptionManager * Fix clippy * Add comment about trace task * Use broadcast instead of mpsc for l2 blocks events * Fix typo (#1246) * Structured Concurrency: Part 2 (#1216) * Implement managed tasks for SubscriptionManager * Fix clippy * Add comment about trace task * Use broadcast instead of mpsc for l2 blocks events * Fix spawns in DaService * Add TaskManager * Use task manager in sequencer * Document TaskManager * Handle shutdown event * Use TaskTracker * Add comment about using a cancellation token * Use JoinHandles instead of TaskTracker * Use TaskManager in fullnode and prover * Improve bitcoin-da service * Force spawned tasks to accept a cancellation token * Use biased polling * Satisfy clippy * Address PR feedback * Fix checks * Pin foundry (#1253) * Pin foundary * Add comment * Remove puclihs mock da block script * Use default block time on error (#1244) * Use default block time on error * Constant target_block_time * Remove import * Dump logs and cleanup on assert failures (#1252) * Fix estiamte gas l1 fee issue when metamask max amount is selected (#1261) * Fix estiamte gas l1 fee issue when metamask max amount is selected * Fix tests * Remove unnecessary comment * Fix hive (#1263) * Fix configs * Remove publish mock block from docker * Update port for hive * Rename config in hive * Refactor eth estimate gas l1 fee issue (#1264) * Refactor eth estimate gas l1 fee issue * Nits * Fix bug * Refactor * Do not ignore resources * Fix --------- Co-authored-by: Roman Proskuryakoff <[email protected]> * Pruning skeleton (#1229) * Implement managed tasks for SubscriptionManager * Fix clippy * Add comment about trace task * Use broadcast instead of mpsc for l2 blocks events * Fix spawns in DaService * Add TaskManager * Use task manager in sequencer * Document TaskManager * Handle shutdown event * Use TaskTracker * Add comment about using a cancellation token * Use JoinHandles instead of TaskTracker * Use TaskManager in fullnode and prover * Improve bitcoin-da service * Force spawned tasks to accept a cancellation token * Use biased polling * Satisfy clippy * WIP * Add pruning tables * Pruning skeleton implementation * Use pruner in nodes * Use biased polling based on order * WIP * Fix how config is done * Derive default * Add logs * Let the tasks finish without panicing * Use pruning config in fullnode and prover * Add simple run test * Use option instead of PruningMode * Unneccessary changes * l2_receiver * Cleanup prints * Use last pruned block in calculation * Implement pruning criteria * Lint and add comment * Set the last_pruned_block to up_to_block value * Don't store config internally * Remove from constructor * Should not change * Move config value * Remove pruning from sequencer / prover * Derive SequencerClient (#1269) * Derive SequencerClient * Renames * Lint * Improve estimate gas and create access list rpcs (#1265) * Remove unnecessary transact_to conversion * unwrap_or_default * More concise block_env initialization * Actually get highest gas limit from request and block env * One liner set * Allow unused * Cleanup prepare_call_env * Fix tests & lint * Replace unwrap * Set state to block in eth_call * Consume request in prepare_call_env * Lint * Set state before reading config * Remove unnecessary clone * Replace allow unused with feature native gate * Add sys txs to evm tests (#1255) * modify config_push_contracts * modify call_multiple_test * modify tests in call_tests.rs * moving common functions to utils.rs * minor fixes * updated lock files * modify tests --------- Co-authored-by: Esad Yusuf Atik <[email protected]> * make prover config arg not optional (#1278) * Pin to 27.1 (#1279) * E2E tests using citrea-e2e (#1277) * E2E tests using citrea-e2e * Lint * Set CITREA path * With github.workspace * Target citrea-e2e main * Update citrea-e2e rev * Use debug build * Update citrea-e2e * Prover generate input rpc (#1280) * WIP Implement prover generate rpcg * It compiles but stf is modified * Context * Fix Context: Send * Merge fix * Move function from common to prover * Remove code duplicate and unnecessary log * Add optional parameter to break commitments into groups * Return input as string --------- Co-authored-by: Roman Proskuryakoff <[email protected]> * Move node configs to citrea-common (#1286) * move node configs to a seperate crate * fix udeps * remove native feature * move SequencerConfig * move config to citrea-common * fix use statements * Update e2e test framework and fix tests (#1305) * Update e2e test framework and fix tests * Fix bug * Update ci binary env key * Test if new fix works * Update bitcoincore-rpc version * Dprint * Try against fix prover config rev * Target main HEAD rev --------- Co-authored-by: jfldde <[email protected]> * build and publish a new image for every commit to the nightly branch (#1309) * Get fee recommendation from mempool space (#1302) * Get fee recommendation from mempool space * Fix bug * Construct mempool space endpoint by network --------- Co-authored-by: Esad Yusuf Atik <[email protected]> * new path fix for nightly (#1310) * Enable pending block tag in simulation endpoints (#1303) * treat pending tag same as latest * fix lint * return new sealed block for pending * handle pending tag externally * revert enabling pending in some endpoints * get blockenv instead of sealed block * address review comments * implement tests for eth_call, eth_estimateGas, eth_createAccessList pending blocks * rename tests --------- Co-authored-by: eyusufatik <[email protected]> * Implement state and block overrides (#1270) * Implement state and block overrides * Update comment * apply_block_overrides func * apply_state_overrides func * Use a single func for replacing account storage * Clippy * Address feedback Use Reth's BlockOverrides with saturating_to to convert back to u64 * Test for block overrides * Test for state overrides * Comment tests * Create a fresh working set * Remove box * remove alloy-serde --------- Co-authored-by: eyusufatik <[email protected]> * Use spawn_blocking for da queue (#1311) * Fetch smart fee only if none (#1312) * update run doc and changelog (#1315) --------- Co-authored-by: Rakan Al-Huneiti <[email protected]> Co-authored-by: Ahmet Yazıcı <[email protected]> Co-authored-by: Erce Can Bektüre <[email protected]> Co-authored-by: jfldde <[email protected]> Co-authored-by: Erce Can Bektüre <[email protected]> Co-authored-by: Roman Proskuryakoff <[email protected]> Co-authored-by: Ege Okan Ünaldı <[email protected]> Co-authored-by: Çetin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Metamask initially sends an estimate_gas request with 0 value, due to there is no balance transfer diff size of balance transfer is not calculated and added to the fee, thus results in a rather low gas estimated. This causes an issue when we select the max amount possible to transfer, so this pr updates estimate_gas so that if value is 0 it adds balance transfer diff size l1 fee to the total l1 fee and calculates the estimated gas accordingly. With this we can use the max amount button in metamask
Linked Issues
eth_estimateGasRPC #693