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

Add effectiveGasPrice to eth_getTransactionReceipt #206

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

lightclient
Copy link
Member

@lightclient lightclient commented Jun 1, 2021

After some discussions, it appears most prefer to add effectiveGasPrice to the receipt object because it is a computed value, like gasUsed. I've written the method with the idea that legacy transactions will simply return tx.gas_price for the effectiveGasPrice and EIP-1559 txs will actually compute the correct value. This should avoid a scenario where RPC consumers need to branch depending on the tx type to determine the effective cost.

cc: @MicahZoltu @tkstanczak @timbeiko @GregTheGreek @fvictorio

@lightclient lightclient marked this pull request as ready for review June 2, 2021 15:57
@lightclient
Copy link
Member Author

lightclient commented Jun 7, 2021

@timbeiko seems like there hasn't been any push back on this. Do you want to go ahead and merge or should we bring it up briefly in ACD?

EDIT: sorry, we should probably discuss on ACD briefly. I see geth uses the effectiveGasPrice as the gasPrice for mined txs.

@lightclient
Copy link
Member Author

Per ACD, we'll no longer return effectiveGasPrice as gasPrice in London txs. It will only compute and return the value in the receipt object.

@karalabe
Copy link
Member

It's not fully clear to me what gasPrice should return now for 1559 txs? Should it be set to 0? Should it be completely omitted? Seems a bit dangerous to drop straight away and any code depending on gasPrice will insta-break. Should we perhaps keep it synonymous to effectiveGasPrice in the receipt and call it deprecated until people switch over?

@karalabe
Copy link
Member

Also worth noting that digging up a receipt is expensive and if you only want to get the price paid, it's a bit of an overkill to do a database lookup just to get a field that doesn't even need any data whatsoever from the receipt.

@MicahZoltu
Copy link
Contributor

MicahZoltu commented Jun 17, 2021

I like dropping it. We will likely continue to have more and more new transaction types over time, and not all of them will be able to backfill old properties with reasonable values. If we backfill this time around, people won't write code that is resilient to future transaction changes and they will break in the future rather than today.

Given that there are probably (hopefully) more future Ethereum dapp/integration developers than there are today Ethereum dapp/integration developers, I would rather break today users than future users (e.g., train them now, while it is less damaging).

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