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

Move gas refund calculation from MainnetTransactionProcessor to GasCalculator #8106

Merged
merged 6 commits into from
Jan 14, 2025

Conversation

daniellehrner
Copy link
Contributor

@daniellehrner daniellehrner commented Jan 11, 2025

PR description

Because of EIP-7623 the gas refund calculation will change with the upcoming Prague hard fork. Right now this calculation is hardcoded in MainnetTransactionProcessor. This PR moves this calculation to the GasCalculator, which allows to change the refund calculation with a new hardfork.

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@daniellehrner daniellehrner changed the title Move gas refund calculation from MainnetTransactionProcessor to RefudCalculator Move gas refund calculation from MainnetTransactionProcessor to RefundCalculator Jan 12, 2025
@daniellehrner daniellehrner force-pushed the feat/refund_calculator branch from 6118848 to 0b3870e Compare January 13, 2025 12:27
@daniellehrner daniellehrner marked this pull request as ready for review January 13, 2025 13:21
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Some minor comments.

I have spiked out the complete EIP-7623 and tested against devnet5 spec tests in this PR: #8117

Copy link
Contributor

@siladu siladu Jan 14, 2025

Choose a reason for hiding this comment

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

There's already several pieces of refund logic scattered across the GasCalculators, most recently calculateDelegateCodeGasRefund.

I think it's confusing to have refund logic both in RefundCalculator classes and in the GasCalculator classes, would prefer one or the other.

I have a slight preference for putting this logic into the GasCalculator classes since it's a smaller change than extracting all the other refund logic.

Won't block the PR on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, if it makes sense to make this a very precise type of RefundCalculator with a precise name, then maybe it works. Not sure what that name would be and if this is sufficiently isolated enough from the other refund logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@siladu I think you are right, I've moved the logic to the GasCalculator and got rid of the RefundCalculator

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@daniellehrner daniellehrner force-pushed the feat/refund_calculator branch from 0b3870e to 894d7b2 Compare January 14, 2025 22:31
@daniellehrner daniellehrner changed the title Move gas refund calculation from MainnetTransactionProcessor to RefundCalculator Move gas refund calculation from MainnetTransactionProcessor to GasCalculator Jan 14, 2025
@daniellehrner daniellehrner enabled auto-merge (squash) January 14, 2025 22:53
@daniellehrner daniellehrner merged commit 42b26a4 into hyperledger:main Jan 14, 2025
43 checks passed
@daniellehrner daniellehrner deleted the feat/refund_calculator branch January 15, 2025 06:27
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