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

Adding fee computation functions to l1BlockInfoTx #134

Merged
merged 12 commits into from
Oct 1, 2024

Conversation

cody-wang-cb
Copy link
Contributor

Motivation

We want to reuse op-alloy's block info type in revm, but it's missing some fee computation functions. Thus this PR aims to add these functionalities.

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

crates/protocol/src/block_info.rs Outdated Show resolved Hide resolved
crates/protocol/src/utils.rs Show resolved Hide resolved
crates/protocol/src/block_info.rs Outdated Show resolved Hide resolved
crates/protocol/src/block_info.rs Outdated Show resolved Hide resolved
crates/protocol/src/block_info.rs Outdated Show resolved Hide resolved
crates/protocol/src/block_info.rs Outdated Show resolved Hide resolved
@cody-wang-cb
Copy link
Contributor Author

Hey @clabby, I just updated the PR to include a new module with the fee calculation functions, PTAL again

One question (related to #134 (comment)):
Should L1BlockInfoTx enums still implement method calculate_tx_l1_cost and call those functions? One thing I’m still not sure about is the edge case around the first block for ecotone needs to use calculate_tx_l1_cost_bedrock calculation, which requires l1_fee_overhead. So in order to implement calculate_tx_l1_cost for L1BlockInfoTxEcotone it seems like it must add this attribute?

crates/protocol/src/fee.rs Outdated Show resolved Hide resolved
crates/protocol/src/fee.rs Outdated Show resolved Hide resolved
@clabby
Copy link
Collaborator

clabby commented Oct 1, 2024

One thing I’m still not sure about is the edge case around the first block for ecotone needs to use calculate_tx_l1_cost_bedrock calculation, which requires l1_fee_overhead. So in order to implement calculate_tx_l1_cost for L1BlockInfoTxEcotone it seems like it must add this attribute?

I think they should not. The consumer can decide to handle this case; i.e. in revm, we'll check if there's empty scalars like we do today, and then choose which of these functions we use as a result.

@cody-wang-cb
Copy link
Contributor Author

@clabby just addressed the comments and all the checks passed now, PTAL again

Copy link
Collaborator

@refcell refcell left a comment

Choose a reason for hiding this comment

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

This is looking good to me pending @clabby's review

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

lgtm, would be nice if you can open an issue to use these to integrate/replace equivalent logic in reth_optimism_evm::l1

@emhane emhane added this pull request to the merge queue Oct 1, 2024
Merged via the queue into alloy-rs:main with commit 071815b Oct 1, 2024
19 checks passed
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.

4 participants