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.

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

refactor(primitives): group effective_gas_tip and effective_tip_per_gas functions together #5141

Closed
tcoratger opened this issue Oct 23, 2023 · 4 comments
Assignees
Labels
C-enhancement New feature or request

Comments

@tcoratger
Copy link
Contributor

tcoratger commented Oct 23, 2023

Describe the feature

Observation

In the current codebase, we have two separate functions, effective_gas_tip and effective_tip_per_gas, in the implementation of the Transaction enum, that perform similar computations to determine the effective gas tip or gasTipCap for a given transaction and base fee:

// TODO: dedup with effective_tip_per_gas
/// Determine the effective gas limit for the given transaction and base fee.
/// If the base fee is `None`, the `max_priority_fee_per_gas`, or gas price for non-EIP1559
/// transactions is returned.
///
/// If the `max_fee_per_gas` is less than the base fee, `None` returned.
pub fn effective_gas_tip(&self, base_fee: Option<u64>) -> Option<u128> {
if let Some(base_fee) = base_fee {
let max_fee_per_gas = self.max_fee_per_gas();
if max_fee_per_gas < base_fee as u128 {
None
} else {
let effective_max_fee = max_fee_per_gas - base_fee as u128;
Some(std::cmp::min(effective_max_fee, self.priority_fee_or_price()))
}
} else {
Some(self.priority_fee_or_price())
}
}
/// Returns the effective miner gas tip cap (`gasTipCap`) for the given base fee:
/// `min(maxFeePerGas - baseFee, maxPriorityFeePerGas)`
///
/// Returns `None` if the basefee is higher than the [Transaction::max_fee_per_gas].
pub fn effective_tip_per_gas(&self, base_fee: u64) -> Option<u128> {
let base_fee = base_fee as u128;
let max_fee_per_gas = self.max_fee_per_gas();
if max_fee_per_gas < base_fee {
return None
}
// the miner tip is the difference between the max fee and the base fee or the
// max_priority_fee_per_gas, whatever is lower
// SAFETY: max_fee_per_gas >= base_fee
let fee = max_fee_per_gas - base_fee;
if let Some(priority_fee) = self.max_priority_fee_per_gas() {
return Some(fee.min(priority_fee))
}
Some(fee)
}

These two functions car be grouped together using a common logic.

Proposed Solution

I propose merging the logic of these two functions into a single function, which we can name effective_tip_per_gas. This consolidated function can accept an Option<u64> for the base fee, making it versatile enough to handle cases where the base fee is either present or absent.

The consolidated function will centralize the logic for calculating the effective gas tip or gasTipCap, which includes handling scenarios where the base fee is not specified. This refactoring will lead to more maintainable and cleaner code.

Additional context

No response

@DoTheBestToGetTheBest
Copy link
Contributor

@mattsse oh i'm sorry i made a PR before you assign tcoratger

@tcoratger
Copy link
Contributor Author

@mattsse oh i'm sorry i made a PR before you assign tcoratger

@DoTheBestToGetTheBest No worries, I hadn't started it and I have others to do so really no problem.

@DoTheBestToGetTheBest
Copy link
Contributor

@mattsse oh i'm sorry i made a PR before you assign tcoratger

@DoTheBestToGetTheBest No worries, I hadn't started it and I have others to do so really

Thank you lot !

@onbjerg
Copy link
Member

onbjerg commented Dec 15, 2023

Solved in #5141

@onbjerg onbjerg closed this as completed Dec 15, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Reth Tracker Dec 15, 2023
@DaniPopes DaniPopes removed the S-needs-triage This issue needs to be labelled label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

4 participants