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

Require gasPrice field for both legacy and EIP-1559 transactions #251

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

lightclient
Copy link
Member

@lightclient lightclient commented Jul 9, 2021

What was wrong?

As discussed in ACD 117, a lot of tooling relies on the gasPrice field of the Transaction object. Although this field doesn't make much sense with EIP-1559 transactions, for the sake of backwards compatibility we should continue supporting it. Please note that this field is DEPRECATED for EIP-1559 transactions, so developers should immediately begin transitioning to using the effectiveGasPrice field on the Receipt object.

How was it fixed?

Return the effectiveGasPrice as the gasPrice field for EIP-1559 transactions. If the transaction has not yet been mined, clients should return the maxFeeCapPerGas value as gasPrice.

Cute Animal Picture

@lightclient
Copy link
Member Author

Please note, after the London hard fork we'll be coordinating JSON-RPC spec changes over here: https://github.com/ethereum/eth1.0-apis

@lightclient
Copy link
Member Author

@timbeiko before I had write access you consistently reviewed and merged the JSON-RPC changes. Let me know if you want to continue doing that or if you prefer I just maintain it.

@timbeiko
Copy link
Collaborator

@lightclient happy for you to merge, but it'd be good to at least get a 👍 from another client dev IMO. Even if they can't actually "Review" on GH.

"gasPrice": {
"title": "transactionGasPrice",
"type": "string",
"description": "The effective gas price paid by the sender in Wei. For transactions not yet mined, this value should be set equal to the max fee cap per gas. This field is DEPRECATED, please transition to using effectiveGasPrice in the Receipt object going forward."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "The effective gas price paid by the sender in Wei. For transactions not yet mined, this value should be set equal to the max fee cap per gas. This field is DEPRECATED, please transition to using effectiveGasPrice in the Receipt object going forward."
"description": "The effective gas price paid by the sender in attoeth. For transactions not yet mined, this value should be set equal to the max fee cap per gas. This field is DEPRECATED, please transition to using effectiveGasPrice in the Receipt object going forward."

I figure it is worth a shot...

Copy link
Member Author

Choose a reason for hiding this comment

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

haha

Copy link

@shanejonas shanejonas Jul 27, 2021

Choose a reason for hiding this comment

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

this could be refactored to change the oneOf wrapping it to be an anyOf

"gasPrice": {
"title": "transactionGasPrice",
"type": "string",
"description": "The effective gas price paid by the sender in Wei. For transactions not yet mined, this value should be set equal to the max fee cap per gas. This field is DEPRECATED, please transition to using effectiveGasPrice in the Receipt object going forward."
Copy link
Contributor

Choose a reason for hiding this comment

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

We should include a clear end of life (EOL) policy here. I suggest something like "This field will no longer be returned by clients sometime after 2022-01-01."

Choose a reason for hiding this comment

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

I'd change that to may no longer be returned. And in practice I would probably recommend returning it indefinitely, since this will be a breaking change.

@MicahZoltu
Copy link
Contributor

MicahZoltu commented Jul 16, 2021

I'm generally against this PR. I suspect it is going to cause more problems than it solves.

By omitting the field entirely, tools will fail but they will fail fast and hard and cleanly and the fix becomes immediately obvious. By including the field but having it mean two different things depending on context, tools are likely to experience weird and hard to debug bugs that aren't easy to reproduce and whose behavior changes depending on when you look at things. This means you'll have a long tail of bugs that will take a long time to get fixed rather than a short burst of tooling bugs that are rapidly fixed.

I think a preferable option for temporary backward compatibility would be to set gasPrice to the transaction's maxFeePerGas. This is a reasonable value that has a well defined meaning and it doesn't change based on the lifecycle of the transaction. If we go this route, we should still deprecate and EOL this field with a clear EOL date, but at least any bugs that come up can much more easily be fixed.

@alcuadrado
Copy link
Member

Any updates on this?

We released our (opt-in) London support in Hardhat yesterday, which includes gasPrice for EIP-1559 txs, as described here. We'd be fine with removing it if this PR is not accepted. We'd rather do that before the Mainnet activation, as we'll make London the default hardfork in Hardhat at the same time, and removing it later would be a larger breaking change.

@lightclient
Copy link
Member Author

What was the decision on ACD @timbeiko? Are we waiting for the infra call this week?

@timbeiko
Copy link
Collaborator

@lightclient we agreed to keep the field in 1559-style transactions as well, have it return maxFeePerGas before being mined and the effectiveGasPrice after. We will deprecate this field right after the upgrade following London.

@MicahZoltu
Copy link
Contributor

@timbeiko That wasn't my takeaway from the meeting... I thought the agreement was to wait and talk to dapp developers in the upcoming dapp integration call and then make a decision after that? Some discussions in the Ethereum R&D Discord seem to imply that at least some dapp developers do want something that won't break everything instantly (so they want a gasPrice field) but they are concerned with the current Geth solution of including a value that changes when a block is mined and a reorg happens.

@timbeiko
Copy link
Collaborator

@MicahZoltu now that you mention it, we did saw we'd bring this up then! My bad. It's on the agenda for this Friday.

@lightclient
Copy link
Member Author

Per the EIP-1559 coordination call #11, tooling devs do prefer the gas price to continue being an element on the transaction object. They weren't against deprecation later on, but no clear path to that was agreed upon on the call.

Since this has been outstanding for about a month and there has been no unresolved push back, I will merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: JSON RPC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants