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 MessageCallGas documentation #904

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

trocher
Copy link

@trocher trocher commented Mar 11, 2024

What was wrong?

  • MessageCallGas.cost's documentation stated that it was "The non-refundable portion of gas reserved for executing the call opcode.". However, MessageCallGas.cost includes the gas to be passed to the sub-call (excluding the 2300 gas stipend) and hence, it is not true that it is non-refundable since some of the gas passed might be refunded upon returning from the sub-call.

  • The name MessageCallGas.stipend can be misleading as "stipend" is also often used to refer to the 2300 gas stipend appended to calls with a non-null value.

How was it fixed?

  • Updated the documentation of MessageCallGas.cost to remove the "non-refundable" mention.
  • Renamed MessageCallGas.stipend to MessageCallGas.sub_call.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

Copy link
Collaborator

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Your analysis seems sound; cost must contain the total cost of the instruction. See:

charge_gas(evm, message_call_gas.cost + extend_memory.cost)

The portion of gas available to sub-calls that is refundable
if not consumed
if not consumed.
"""

cost: Uint
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to also rename cost to total or something similar? Looks like it excludes the memory expansion cost though, and that might be a tad misleading.

Copy link
Author

Choose a reason for hiding this comment

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

  • Added in a comment that it excludes memory expansion to be explicit.
  • Keeping cost in the variable name is good imo since it is added to some other cost afterwards, maybe total_cost would work?

charge_gas(evm, message_call_gas.cost + extend_memory.cost)

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.

2 participants