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

improve calculate_next_block_base_fee doc #5135

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

tcoratger
Copy link
Contributor

The calculate_next_block_base_fee function has until now included quite a few formulas and nested logic with little documentation. I thought it might be quite hard to read for a newcomer. So I added several improvements (in my opinion), don't hesitate to tell me if some are not suitable:

  • I added a slightly more extensive comment doc at the start of the function to briefly explain what it does and the link with EIP-1559.
  • I divided if / else if / else into three with documentation each time to understand the logic related to the gas_target.
  • In each statement, I refactored the operations a little to be able to read them as a single operation and I favored multiplication rather than two successive divisions (normally in terms of performance it goes a little faster if I'm not mistaken, so it's always winning even if it's very minor).

@codecov
Copy link

codecov bot commented Oct 22, 2023

Codecov Report

Merging #5135 (5ad06e5) into main (a6bee72) will decrease coverage by 0.12%.
Report is 13 commits behind head on main.
The diff coverage is 93.33%.

Impacted file tree graph

Files Coverage Δ
crates/primitives/src/basefee.rs 98.11% <93.33%> (-0.07%) ⬇️

... and 32 files with indirect coverage changes

Flag Coverage Δ
integration-tests 17.07% <0.00%> (-0.02%) ⬇️
unit-tests 62.13% <93.33%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 30.85% <ø> (ø)
blockchain tree 80.82% <ø> (ø)
pipeline 88.36% <ø> (ø)
storage (db) 74.60% <ø> (ø)
trie 95.00% <ø> (+0.06%) ⬆️
txpool 55.31% <ø> (-0.03%) ⬇️
networking 78.16% <ø> (-0.03%) ⬇️
rpc 57.80% <ø> (-0.69%) ⬇️
consensus 63.01% <ø> (ø)
revm 24.47% <ø> (+0.28%) ⬆️
payload builder 7.95% <ø> (ø)
primitives 84.72% <93.33%> (-0.12%) ⬇️

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.

lgtm, pending @Rjected

@mattsse mattsse requested a review from Rjected October 23, 2023 11:30
@mattsse mattsse added the C-docs An addition or correction to our documentation label Oct 23, 2023
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

LGTM, I like the docs a lot, the basefee method looks equivalent

@mattsse mattsse added this pull request to the merge queue Oct 23, 2023
Merged via the queue into paradigmxyz:main with commit 1eb5f41 Oct 23, 2023
23 checks passed
mattsse pushed a commit that referenced this pull request Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-docs An addition or correction to our documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants