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

rework priority fee logic #33

Open
wants to merge 3 commits into
base: hyperlane
Choose a base branch
from

Conversation

christianangelopoulos
Copy link

Motivation

updated fee logic calculation to reflect latest changes in Alloy.rs which uses EIP1559_FEE_ESTIMATION_DEFAULT_PRIORITY_FEE of 1 wei instead of 100,000,000 wei (0.1 Gwei).

should save users of hyperlane relayer a substantial amount of gas on L2s where the base gas fee can be thousands of times lower than 0.1 Gwei.

Here is an Example transaction where priority fee was set to 0.1 Gwei

Solution

  • Updated EIP1559_FEE_ESTIMATION_DEFAULT_PRIORITY_FEE to 1 wei.
  • Added EIP1559_BASE_FEE_MULTIPLIER and changed fn base_fee_surged to use it in order to better reflect recent estimator in Alloy.

@christianangelopoulos
Copy link
Author

FYI - test_eip1559_default_estimator was failing previous to these changes. Now that it works, a bunch of other downstream tests are failing, that appear to be unrelated to these changes (when I comment out the test that was failing in the previous commit, I get the same failures)

Copy link

@aroralanuk aroralanuk left a comment

Choose a reason for hiding this comment

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

looking at this

Copy link

@aroralanuk aroralanuk left a comment

Choose a reason for hiding this comment

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

Great catch, a couple of points:

  • can remove the if max_priority_fee_per_gas > potential_max_fee branch and just add base to priority fee, bc with the status quo, one edge case is if potential_max_fee == max_priority_fee_per_gas, we end by (x,x) value so effective making base fee 0 which will get stuck
  • similar to alloy, can add EIP1559_MIN_PRIORITY_FEE if rewards.is_empty() in case some chains don't accept zero values

// changing to reflect https://github.com/alloy-rs/alloy/blob/1060b08ffc4ce5b858755dec15da34a4ccf43d0f/crates/provider/src/utils.rs#L44
base_fee_per_gas * U256::from(EIP1559_BASE_FEE_MULTIPLIER)

// if base_fee_per_gas <= U256::from(40_000_000_000u64) {

Choose a reason for hiding this comment

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

Can you remove the commented-out code below?

Choose a reason for hiding this comment

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

Great point on edge case. Addressed the above:

  • removed the if max_priority_fee_per_gas > potential_max_fee branch to reflect the same logic as alloy
  • removed commented code

similar to alloy, can add EIP1559_MIN_PRIORITY_FEE if rewards.is_empty() in case some chains don't accept zero values

this is covered by this this part, unless I'm missing something

    let max_priority_fee_per_gas = std::cmp::max(
        estimate_priority_fee(rewards),
        U256::from(EIP1559_FEE_ESTIMATION_DEFAULT_PRIORITY_FEE),
    );

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.

2 participants