Skip to content

Comments

chore: add ensure_enough_balance helper#3033

Merged
rakita merged 1 commit intomainfrom
rakita/ensure-enough-balance
Oct 1, 2025
Merged

chore: add ensure_enough_balance helper#3033
rakita merged 1 commit intomainfrom
rakita/ensure-enough-balance

Conversation

@rakita
Copy link
Member

@rakita rakita commented Sep 30, 2025

Helper function for ensure_enough_balance for Transaction that compares balance with max amount that tx could spend.

Extracted from #2991

@rakita rakita requested review from klkvr and mattsse September 30, 2025 21:50
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 30, 2025

CodSpeed Performance Report

Merging #3033 will degrade performances by 3.49%

Comparing rakita/ensure-enough-balance (597f39a) with main (6e29b0c)

Summary

❌ 1 regression
✅ 172 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
transfer 11.9 µs 12.3 µs -3.49%

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

makes sense but unsure about the op changes


let max_balance_spending = tx.max_balance_spending()?;

// Check if account has enough balance for `gas_limit * max_fee`` and value transfer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we keep this doc?

.into());
if !is_deposit && !is_balance_check_disabled {
// check additional cost and deduct it from the caller's balances
let Some(balance) = new_balance.checked_sub(additional_cost) else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah so this was moved inside here replacing max_balance_spending > new_balance ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the deposit and the rest of the transaction overlapped a lot, and it wasn't very clear to look.

max_balance_spending > new_balance is now done in ensure_enough_balance and additional_cost is checked first before ensure_ is called

Copy link
Member Author

@rakita rakita Oct 1, 2025

Choose a reason for hiding this comment

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

And additional_cost is not applicable for deposits, it is zero for it

)?;
}

let max_balance_spending = tx.max_balance_spending()?.saturating_add(additional_cost);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we no longer chheck that this doesn't overflow for all txs, is this right?

Copy link
Member Author

@rakita rakita Oct 1, 2025

Choose a reason for hiding this comment

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

Yes, it was never checked. Deposit tx skips this check.

@rakita rakita merged commit a32f8f4 into main Oct 1, 2025
30 of 31 checks passed
@rakita rakita deleted the rakita/ensure-enough-balance branch October 1, 2025 18:41
@github-actions github-actions bot mentioned this pull request Oct 5, 2025
theochap pushed a commit to ethereum-optimism/optimism that referenced this pull request Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants