You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.20%. Comparing base (f959eb9) to head (ae4b09d). ⚠️ Report is 4 commits behind head on develop.
PR Review: fix(rpc): align eth_feeHistory response with execution-apis spec
Gather context and read changed files
Analyze code changes
Post comprehensive review
Summary
This PR makes two changes to eth_feeHistory:
Silently cap blockCount when it exceeds FeeHistoryCap, instead of returning an error.
Add baseFeePerBlobGas and blobGasUsedRatio fields (EIP-4844 blob fields) to the response, populated with zeros since Ethermint doesn't support blob transactions.
The previous behavior returned an error when blocks > maxBlockCount. The new behavior silently caps. This is spec-compliant (go-ethereum caps at 1024 without erroring), but it is a breaking change for clients that relied on the error response to detect invalid or over-limit requests.
The PR description should clearly document this behavioral change. Consider adding a note in the changelog that this is a breaking client-facing behavior change.
Edge Case: FeeHistoryCap = 0 Produces a Nonsensical Response
The test at rpc/backend/chain_info_test.go:350 shows that when FeeHistoryCap = 0 and userBlockCount = 1:
blocks gets capped to 0
oldestBlock becomes blockNumber + 1 = 2, a block that doesn't exist yet
The response returns an empty history with a future oldestBlock
This is a confusing but pre-existing invariant exposed by the silent-cap change. With the old error-returning behavior, FeeHistoryCap = 0 would immediately return an error for any non-zero blockCount. With silent capping, callers silently receive a degenerate response. If FeeHistoryCap = 0 is a valid configuration to mean "no fee history", a zero guard could help:
GasUsedRatio has no omitempty (always serialized), while BlobGasUsedRatio has omitempty. Since the code always initializes BlobGasUsedRatio with a non-nil slice (make([]float64, blocks)), the omitempty tag on BlobGasUsedRatio has no effect in practice (Go's encoding/json only omits slices when they are nil, not when they are empty). This is a documentation/intention mismatch — if the intent is to allow omitting the field for pre-Cancun nodes in the future, a comment explaining this would help; otherwise, removing omitempty for consistency would be cleaner.
Blob Fields Always Return Zeros
thisBlobBaseFee and thisBlobGasUsedRatio are always initialized with zero values. Since Ethermint doesn't implement EIP-4844, this is correct for now. However, there is no comment explaining why, which could confuse future contributors. A short comment such as // EIP-4844 blob transactions are not supported; return zeros per spec would be helpful.
Positive Aspects
Spec compliance: JSON field names baseFeePerBlobGas and blobGasUsedRatio exactly match the execution-apis spec.
Silent cap aligns with go-ethereum's behavior.
Test coverage is updated for all changed cases; Codecov confirms 100% coverage of modified lines.
CHANGELOG entry is present.
Verdict
The changes are correct and improve spec compliance. The main concerns are:
The silent-cap behavioral change should be explicitly noted (it's a client-facing API behavior change).
The FeeHistoryCap = 0 edge case produces a counterintuitive response with a future oldestBlock.
PR Review: fix(rpc): align eth_feeHistory response with execution-apis spec
Gather context and read changed files
Analyze code changes
Post comprehensive review
Summary
This PR makes two improvements to eth_feeHistory:
Silent cap: When blockCount exceeds FeeHistoryCap, the value is silently capped instead of returning an error — matching go-ethereum's behavior.
EIP-4844 blob fields: baseFeePerBlobGas and blobGasUsedRatio fields are added to the response (always zeroed since Ethermint doesn't support blob transactions).
No consensus/state changes — this is a pure RPC-layer change and is safe to merge to release branches.
Issues Found
Behavioral Change: Silent Cap Instead of Error
rpc/backend/chain_info.go:216–218
The previous behavior returned an error when blocks > maxBlockCount. The new behavior silently caps. This aligns with go-ethereum's behavior but is a breaking change for any client that relied on the error response to detect over-limit requests. The PR description and changelog entry should explicitly document this behavioral change.
BlobBaseFee has omitempty while BlobGasUsedRatio does not. In Go's encoding/json, omitempty only omits a slice when it is nil — an initialized empty slice ([]*hexutil.Big{}) is always serialized. Since BlobBaseFee is always initialized in this code (either []*hexutil.Big{} in the zero-block path or a fully populated slice), the omitempty tag never triggers in practice. This is not a bug, but it creates an inconsistency between the two new fields. Consider either removing omitempty from BlobBaseFee for consistency, or matching the pattern of BaseFee (which has omitempty) and documenting the intent.
Positive Aspects
Zero-block guard (rpc/backend/chain_info.go:219–227): The if blocks == 0 guard correctly handles both userBlockCount = 0 and the FeeHistoryCap = 0 case, returning a clean empty response rather than the previous behavior that would compute a future oldestBlock.
Correct array lengths: thisBlobBaseFee uses blocks+1 (matching baseFeePerGas per spec) and thisBlobGasUsedRatio uses blocks (matching gasUsedRatio). Both are correct per the execution-apis spec.
Comment added at line 329 explaining why blob fields are hardcoded to zero — clear and helpful for future contributors.
JSON field namesbaseFeePerBlobGas and blobGasUsedRatio match the execution-apis spec exactly.
Test coverage: All changed cases are covered and Codecov reports 100% coverage of modified lines.
CHANGELOG entry is present.
Verdict
The changes are correct, well-tested, and improve spec compliance. The two points above are minor: the behavioral change documentation is a PR description concern, and the omitempty inconsistency has no runtime impact. Neither is blocking.
| Branch: song/fix_eth_feeHistory
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
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.
Closes: #XXX
Description
For contributor use:
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerFor admin use:
WIP,R4R,docs, etc)