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

Rename tx.gas -> tx.gasLimit #2835

Merged
merged 1 commit into from
Aug 1, 2020
Merged

Conversation

minaminao
Copy link
Contributor

It clarifies the meaning of the terms :)

Discussion log:
image

@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

@axic
Copy link
Member

axic commented Jul 29, 2020

Why is this EIP using a mixture of EVM opcodes and Solidity notation? Why not use just EVM opcodes?

Correction, I thought GASPRICE/GASUSED were opcodes, but they are variables introduced in the spec.

Nonetheless, my point is that it is a bad idea relying on Solidity notation in a Core EIP and should only use EVM opcodes, because Solidity may change any given time and its syntax is not governed by EIPs, but EVM opcodes are governed by EIPs.

@MicahZoltu
Copy link
Contributor

Is there an opcode for GAS_LIMIT?

@MicahZoltu
Copy link
Contributor

A quick glance at the yellow paper suggests there isn't an opcode for transaction gas limit. There is one for gas remaining, and one for block gas limit, but not one for transaction gas limit from what I can tell.

@axic
Copy link
Member

axic commented Jul 29, 2020

Solidity can only expose things which have an opcode 😉

The "aptly" named opcode GASLIMIT is the block gas limit, and not transaction gas limit as imagined by many. There is no way to know the transaction gas limit from within the EVM, apart from running GAS as the very first instruction.

@axic
Copy link
Member

axic commented Jul 29, 2020

Btw that's why EIP-1803 proposes to rename those opcodes for clarity.

@MicahZoltu
Copy link
Contributor

While I agree that using a mix of opcodes, definitions, and solidity is bad, I still think this PR is an iterative improvement over the previous state of things so I stand by my approval of it.

As far as how to improve things further, I think the best option is to just have every term be added to the definitions list in the specification. In this case, we would need to add a TRANSACTION_GAS_LIMIT (or similar name).

@axic
Copy link
Member

axic commented Jul 29, 2020

I should have read the EIP carefully before commenting. I agree there's no need to even use EVM opcodes here as this is functionality outside of the EVM. And this also means using Solidity notiation is very confusing to the reader.

I'd agree to use only definitions and avoid EVM/Solidity here.

@axic
Copy link
Member

axic commented Jul 29, 2020

As for merging this, I think the actual authors shall approve and merge, and it should not be forcefully merged by an editor. Shall the authors not want to merge this, the disagreement with the notation can still be voiced during the last call process.

@MicahZoltu
Copy link
Contributor

As for merging this, I think the actual authors shall approve and merge, and it should not be forcefully merged by an editor. Shall the authors not want to merge this, the disagreement with the notation can still be voiced during the last call process.

Totally agree, the only reason I approved it is because I have been pretty active with 1559 and I wanted to signal my agreement. It was an approval from Micah EIP Author not Micah EIP Editor. 😄

@AFDudley
Copy link
Contributor

This change moves the EIP terminology away from what's currently in the code, right?

@MicahZoltu
Copy link
Contributor

This change moves the EIP terminology away from what's currently in the code, right?

What code? Besu and Geth you mean? I don't know what either are using for internal variable naming, but it feels like internal naming conventions for clients shouldn't play a role in deciding how to best word an EIP?

@minaminao
Copy link
Contributor Author

Sorry to interrupt you. Maybe we should make the following case style changes for consistency:

  • tx.GasPremium -> tx.gasPremium
  • tx.fee_cap -> tx.feeCap

Copy link
Contributor

@i-norden i-norden left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'll clean up the geth terminology during the next code iteration. Relevant code for geth is here, the terms are confused in the message type.

@eip-automerger eip-automerger merged commit ab5191b into ethereum:master Aug 1, 2020
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
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.

6 participants