refactor: remove PragueFeeMarket
#8664
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
There was a problem hiding this comment.
Pull Request Overview
This PR removes the now-redundant PragueFeeMarket, unifies it under the new BlobFeeMarket, and updates all related factories, interfaces, and tests.
- Renamed
CancunFeeMarkettoBlobFeeMarket(always requires abaseFeeUpdateFraction) and removedPragueFeeMarket. - Merged the two per-fork fee market tests into one parameterized
BlobFeeMarketTest, and updated all other tests to callimplementsBlobFee()andFeeMarket.cancunDefault(...). - Introduced
FeeMarketBuilderFactory.createFeeMarket(...)and refactoredMainnetProtocolSpecsto use it.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| .../ZeroBaseFeeMarketTest.java | Renamed test to implementsBlobFeedShouldReturnFalse |
| .../FixedBaseFeeMarketTest.java | Same rename for fixed base fee market test |
| .../BlobFeeMarketTest.java | Added new parameterized test over BlobSchedule |
| .../FeeMarket.java | Replaced implementsDataFee() with implementsBlobFee(), added new static constructors |
| .../FeeMarketBuilderFactory.java | New factory class for fee market builders |
| .../MainnetProtocolSpecs.java | Refactored to use createFeeMarket instead of inline builders |
Comments suppressed due to low confidence (2)
ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/feemarket/ZeroBaseFeeMarketTest.java:144
- [nitpick] The test method name says
implementsBlobFeedbut the interface method isimplementsBlobFee(). Renaming it toimplementsBlob 8000 FeeShouldReturnFalsewill keep it consistent.
public void implementsBlobFeedShouldReturnFalse() {
ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/feemarket/FixedBaseFeeMarketTest.java:144
- [nitpick] The test name uses
BlobFeedinstead ofBlobFee; consider renaming toimplementsBlobFeeShouldReturnFalseto match the API.
public void implementsBlobFeedShouldReturnFalse() {
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetProtocolSpecs.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetProtocolSpecs.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/feemarket/FeeMarket.java
Show resolved
Hide resolved
...um/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/feemarket/BlobFeeMarketTest.java
Show resolved
Hide resolved
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the fee market implementation by removing the redundant PragueFeeMarket class and unifying fee market behavior under the BlobFeeMarket. It also updates associated tests and protocol specification builders to use the new BlobFeeMarket and the cancunDefault factory method.
- Removed PragueFeeMarket and its tests.
- Renamed methods from implementsDataFee to implementsBlobFee.
- Updated fee market provisioning in protocol specs and tests via FeeMarketBuilderFactory using cancunDefault.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ZeroBaseFeeMarketTest.java | Renamed test method to check implementsBlobFee. |
| PragueFeeMarketTest.java | Entire file removed as PragueFeeMarket is no longer needed. |
| FixedBaseFeeMarketTest.java | Renamed test method to check implementsBlobFee. |
| CancunFeeMarketTest.java | Removed as part of consolidation with BlobFeeMarket. |
| BlobFeeMarketTest.java | Updated tests to parameterize fee market by BlobSchedule. |
| PragueTargetingGasLimitCalculatorTest.java | Updated fee market creation to include BlobSchedule.PRAGUE_DEFAULT. |
| MainnetTransactionValidatorTest.java, CancunTargetingGasLimitCalculatorTest.java, Eth* tests | Updated fee market instantiation from cancun() to cancunDefault(). |
| FeeMarket.java | Updated fee market factory methods and documentation to reflect new BlobFeeMarket usage. |
| BlobFeeMarket.java | Renamed class and methods updated per new BlobFeeMarket logic. |
| MainnetProtocolSpecs.java | Refactored fee market builder composition using FeeMarketBuilderFactory and cancunDefault. |
| AbstractBlockCreator.java | Updated fee market method reference from implementsDataFee to implementsBlobFee. |
| BlobSchedule.java | Added toString() override for BlobSchedule instances. |
Comments suppressed due to low confidence (1)
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/feemarket/FeeMarket.java:83
- The Javadoc for this overloaded method mentions that the BlobSchedule parameter is ignored; consider clarifying why it is included to avoid confusion for future maintainers.
static BaseFeeMarket london(final long londonForkBlockNumber, final Optional<Wei> baseFeePerGasOverride, final BlobSchedule ignored) {
siladu
approved these changes
May 22, 2025
...e/src/main/java/org/hyperledger/besu/ethereum/mainnet/feemarket/FeeMarketBuilderFactory.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
9 tasks
pinges
approved these changes
May 22, 2025
| static BaseFeeMarket london( | ||
| final long londonForkBlockNumber, final Optional<Wei> baseFeePerGasOverride) { | ||
| return new LondonFeeMarket(londonForkBlockNumber, baseFeePerGasOverride); | ||
| final long londonForkBlockNumber, |
There was a problem hiding this comment.
Can't we get the london fork block number from the genesis?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The
PragueFeeMarketis functionally identical toBlobFeeMarket(formerlyCancunFeeMarket), with the sole distinction being thebaseFeeUpdateFractionparameter, which is determined by theBlobSchedule.Updates
CancunFeeMarkettoBlobFeeMarket.PragueFeeMarketclass, as it is redundant and does not introduce any unique changes.FeeMarketfrom thePragueDefinition, as it is unnecessary. The builder will pass the correctBlobScheduleto the inheritedFeeMarketfrom theBlobFeeMarketdefinition.PragueFeeMarketTestandCancunFeeMarketTestinto a single test class,BlobFeeMarketTestBlobFeeMarket; it will now always expect abaseFeeUpdateFraction.cancunDefaultmethod inFeeMarketto create Blob fee markets with default CancunBlobScheduleparameters, primarily for use in test cases.implementsDataFeemethod toimplementsBlobFee.