fix(rpc): align eth_getBlockReceipts response with execution-apis s…#965
fix(rpc): align eth_getBlockReceipts response with execution-apis s…#965XinyuCRO wants to merge 2 commits into
eth_getBlockReceipts response with execution-apis s…#965Conversation
Signed-off-by: Xinyu <91446598+XinyuCRO@users.noreply.github.com>
|
@claude review PR |
|
Claude finished @XinyuCRO's task in 3m 24s —— View job PR Review:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * (evm) [#921](https://github.com/crypto-org-chain/ethermint/pull/921) fix: enforce floor-data-gas | ||
| * (test) [#926](https://github.com/crypto-org-chain/ethermint/pull/926) fix(test): remove flaky `base_fee` assertion in `update_feemarket_param`. | ||
| * (server) [#946](https://github.com/crypto-org-chain/ethermint/pull/946) feat(server): make JSON-RPC batch limits configurable. | ||
| * (rpc) [#965](https://github.com/crypto-org-chain/ethermint/pull/965) fix(rpc): align `eth_getBlockReceipts` response with execution-apis spec. |
There was a problem hiding this comment.
The PR author has already self-suggested this change in their review comment. Confirming the suggestion:
| * (rpc) [#965](https://github.com/crypto-org-chain/ethermint/pull/965) fix(rpc): align `eth_getBlockReceipts` response with execution-apis spec. | |
| * (rpc) [#965](https://github.com/crypto-org-chain/ethermint/pull/965) fix(rpc): align `eth_getBlockReceipts` response with execution-apis spec. |
|
|
||
| if txData.Type() == ethtypes.DynamicFeeTxType { | ||
| if txData.Type() == ethtypes.BlobTxType { | ||
| receipt["blobGasUsed"] = hexutil.Uint64(0) |
There was a problem hiding this comment.
The blob gas values are hardcoded to 0. Ethermint does not currently support EIP-4844 blob transactions, so the current chain state will never actually produce BlobTxType transactions. However, if blob support is ever added, these hardcoded zeros would silently produce wrong receipts. Consider adding a comment explaining why these are zero (i.e., blobs not supported / always stripped before Cosmos execution), or guarding with a TODO.
| receipt["effectiveGasPrice"] = hexutil.Big(*ethMsg.GetEffectiveGasPrice(baseFee)) | ||
| baseFee = nil | ||
| } | ||
| receipt["effectiveGasPrice"] = hexutil.Big(*ethMsg.GetEffectiveGasPrice(baseFee)) |
There was a problem hiding this comment.
Potential nil dereference. When baseFee fetch fails (pruned node), baseFee is set to nil and then GetEffectiveGasPrice(nil) is called. Looking at GetEffectiveGasPrice:
func (msg MsgEthereumTx) GetEffectiveGasPrice(baseFee *big.Int) *big.Int {
tx := msg.AsTransaction()
if baseFee == nil {
return tx.GasPrice() // safe
}
return ethermint.BigMin(new(big.Int).Add(tx.GasTipCap(), baseFee), tx.GasFeeCap())
}tx.GasPrice() is safe for nil baseFee. However, the outer code does hexutil.Big(*ethMsg.GetEffectiveGasPrice(baseFee)). If GetEffectiveGasPrice returns nil (e.g., if tx.GasPrice() could return nil), this dereference would panic. This is the same pattern pre-existing in the codebase, but worth noting for reviewers that this is safe only as long as go-ethereum guarantees GasPrice() is non-nil.
| return b.TendermintBlockByNumber(*blockNrOrHash.BlockNumber) | ||
| } | ||
| blockNum := rpctypes.EthLatestBlockNumber | ||
| return b.TendermintBlockByNumber(blockNum) |
There was a problem hiding this comment.
The fallback when both BlockHash and BlockNumber are nil silently uses EthLatestBlockNumber. This matches the eth_getBlockByNumber behavior, but for eth_getBlockReceipts it may be surprising. The execution-apis spec requires either a block hash or a number — a fully empty BlockNumberOrHash shouldn't normally reach this function. Consider adding an assertion or debug log here to surface this case if it ever occurs unexpectedly.
|
|
||
| func (suite *BackendTestSuite) TestGetBlockReceipts_ByHash() { | ||
| msgEthereumTx, txBz := suite.buildEthereumTxWithNonceAndGas(0, 45000) | ||
| hash := common.Hash{} |
There was a problem hiding this comment.
The test uses common.Hash{} (the zero hash), which happens to match the hardcoded mock expectation in RegisterBlockByHash for the zero hash. This is fragile — the hash variable is defined but never actually used to look up a real block; it just matches because RegisterBlockByHash hardcodes the zero-byte slice. Consider adding a comment clarifying the test intent: that a non-zero hash would need its own mock registration, and that this test specifically validates the "by hash" code path dispatches correctly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #965 +/- ##
===========================================
+ Coverage 44.15% 44.16% +0.01%
===========================================
Files 196 196
Lines 16896 16908 +12
===========================================
+ Hits 7461 7468 +7
- Misses 8645 8649 +4
- Partials 790 791 +1
🚀 New features to boost your workflow:
|
1 participant
…pec.
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)