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

Update the gas page with an explanation of EIP 1559 #3104

Merged
merged 9 commits into from
Jul 15, 2021
Merged

Update the gas page with an explanation of EIP 1559 #3104

merged 9 commits into from
Jul 15, 2021

Conversation

qbzzt
Copy link
Contributor

@qbzzt qbzzt commented May 22, 2021

Description

Update the gas price section with the details for EIP 1559, as well as a brief explanation of the reason for the change.

Related Issue

#3022

@carlfairclough
Copy link
Contributor

This is a perfect summary IMO :)

Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

@qbzzt Thanks so much for this! Couple small adjustments, but overall looks really good. Should be able to get this in very soon.

src/content/developers/docs/gas/index.md Outdated Show resolved Hide resolved
src/content/developers/docs/gas/index.md Outdated Show resolved Hide resolved
| A | 90 gwei | 90 gwei | N/A | N/A | This transaction is not going in the block |
| B | 200 gwei | 5 gwei | 5 gwei | 105 gwei | The priority fee is the maximum priority fee |
| C | 120 gwei | 30 gwei | 20 gwei | 120 gwei | The priority fee is the maximum (total) fee minus the base fee |
| D | gas price = 200 gwei | gas price = 200 gwei | 100 gwei | 200 gwei | Transactions that specify gas price are charged the full amount |
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to include "gas price = " on only two of these cells?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because only transaction D is old style and actually has a gas price field. I added to the paragraph that introduces the table to clarify.

@samajammin samajammin self-assigned this Jun 1, 2021
Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

@qbzzt Curious if you agree with the comment I left about including a line about the current block size limit doubling, then targeting 50% of this. Also one other suggestion for an edit.

Sorry for the delays on this, but took another look myself and other than small note above I think it looks solid. Personally would like to get this up sooner than later as London is rapidly approaching.

Comment on lines 84 to 85
that compares the size of the previous block (the amount of gas used for all the transactions) with a target size. If the block size is higher than the target, there
is more demand for inclusion in the blockchain than supply so the base fee, is increased. If the block size is lower than the target then there is more supply
Copy link
Member

Choose a reason for hiding this comment

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

If the block size is higher than the target, there is more demand for inclusion in the blockchain than supply so the base fee, is increased.

Perhaps we should note somewhere in here that max block size doubles with this upgrade, and the "target" is 50% of this, allowing for dynamic block sizes.

Consider changing the end of this sentence to (note comma placement too)
"...there is more demand for inclusion in the blockchain than supply, so the base fee in the following block is increased."

@wackerow
Copy link
Member

wackerow commented Jul 7, 2021

@samajammin Have any thoughts on this content update?

https://hackmd.io/@q8X_WM2nTfu6nuvAzqXiTQ/1559-wallets

I found that link to be helpful as a confirmatory resource

@samajammin
Copy link
Member

@wackerow ya I trust your judgement. Agree let's get this content merged sooner rather than later. Good is better than perfect 😀

@netlify
Copy link

netlify bot commented Jul 8, 2021

👷 Deploy request for netlify-edge-ethereum-org pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: c787e12

Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

Currently have an issue with the production site build. Waiting on merges until that's resolved, but made a couple adjustments, and approving this in the meantime =)

Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

👍🏻 Thanks for this @qbzzt!

@github-actions github-actions bot added the translation 🌍 This is related to our Translation Program label Jul 14, 2021
@wackerow wackerow merged commit 8c06df1 into ethereum:dev Jul 15, 2021
@wackerow wackerow mentioned this pull request Jul 15, 2021
@samajammin samajammin mentioned this pull request Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits translation 🌍 This is related to our Translation Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants