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

refactor: enhance readability #968

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

IaroslavMazur
Copy link
Contributor

No description provided.

@gakonst
Copy link
Collaborator

gakonst commented Jan 11, 2024

these seem unnecessary to me, personally

@rakita
Copy link
Member

rakita commented Jan 11, 2024

would agree with @gakonst, gas to change_gas is not needed

@gakonst gakonst closed this Jan 13, 2024
@gakonst
Copy link
Collaborator

gakonst commented Jan 13, 2024

Thanks @IaroslavMazur! Ideally let's try to agree on a change before implementing it, so we can make sure we do not waste your time. Appreciate it!!

@rakita
Copy link
Member

rakita commented Jan 13, 2024

Thanks @IaroslavMazur! Ideally let's try to agree on a change before implementing it, so we can make sure we do not waste your time. Appreciate it!!

@gakonst we are already chatting and will reopen this for the time being.

@rakita rakita reopened this Jan 13, 2024
@IaroslavMazur
Copy link
Contributor Author

@gakonst, @rakita, thank you for your feedback!

As I can understand, the apple of discord here is the gas! -> charge_gas! change. So, let me explain my reasoning for it.

The renaming was my attempt at both making the macro name more suggestive for the less experienced revm contributors/explorers (like me) - and decrease the logical discrepancy between what the macro does and how it's named.

That macro is responsible for recording the gas cost of each executed operation, by subtracting the cost from the gas amount remaining for the tx - and panicking if there's not enough gas left. This is, essentially, "charging" the tx signer "gas" as the tx is being executed. Therefore, the proposed name change.

Besides, while this macro is not a function, it, too, results in some action (which can be suggestively described with a verb) being executed where the macro is referenced in the codebase. "gas", however, is a noun - and this introduces a logical discrepancy between what the macro does - and how it's named.

This discrepancy becomes especially obvious in the context of the gas_or_fail! macro, where "gas" is inaproppriately used as a verb, alongside "fail". This name suggests that the underlying code will either "gas" or "fail", which doesn't make sense.

While the current names of the macros can undoubtedly be mapped in the minds of the revm contributors/explorers to what their underlying code actually does (which is not obvious from their names), ideally, the names should be self-explanatory, if possible (and it is possible in this case, imo).

@IaroslavMazur
Copy link
Contributor Author

After some more talking to @rakita, I've just pushed a patch, removing the changes made to the names of the gas! and gas_or_fail! macros.

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm

@rakita rakita merged commit f5db653 into bluealloy:main Jan 15, 2024
23 checks passed
@github-actions github-actions bot mentioned this pull request Jan 15, 2024
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.

None yet

3 participants