-
Notifications
You must be signed in to change notification settings - Fork 202
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
fix(evm): removed deprecated root key from eth_getTransactionReceipt #2108
Conversation
WalkthroughThe changes in this pull request primarily involve updates to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)CHANGELOG.md (1)
The entry properly describes the removal of the deprecated root key from eth_getTransactionReceipt, links to the PR, and is correctly categorized as a bug fix. This aligns with the PR objectives of resolving Hardhat parsing issues. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
1-24
: Consider standardizing version link formatting.While the changelog follows good practices overall, there are some inconsistencies in how version links are formatted. Some versions use the full GitHub URL format like
[v0.19.4](https://github.com/NibiruChain/nibiru/releases/tag/v0.19.4)
while others use a simpler format like[v1.0.0]
. Consider standardizing to use the full URL format consistently for better accessibility.
Line range hint
25-1200
: Consider improving changelog organization for better readability.A few suggestions to enhance the changelog organization:
- Consolidate duplicate entries that appear in multiple categories
- Consider grouping all dependency updates into a single section per version
- Make entries more concise by focusing on the key changes
- Use consistent formatting for PR links throughout
This would make the changelog easier to navigate and understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.md
(1 hunks)eth/rpc/backend/tx_info.go
(1 hunks)
🧰 Additional context used
📓 Learnings (1)
eth/rpc/backend/tx_info.go (1)
Learnt from: onikonychev
PR: NibiruChain/nibiru#2101
File: eth/rpc/backend/tx_info.go:144-170
Timestamp: 2024-10-31T16:39:44.773Z
Learning: In the file `eth/rpc/backend/tx_info.go`, when implementing the `MarshalJSON` method for `TransactionReceipt`, using a custom struct for marshaling instead of an intermediate `map[string]interface{}` does not work.
🔇 Additional comments (2)
eth/rpc/backend/tx_info.go (1)
170-172
: LGTM! The change correctly removes the deprecated root key.
The removal of the deprecated "root" key aligns with Ethereum's specification and fixes Hardhat deployment issues. The implementation is correct and well-documented.
Let's verify that the "root" key is not required in other Ethereum JSON-RPC responses:
✅ Verification successful
Removal of deprecated "root" key is correct and safe
The verification confirms that:
- The "root" key is only used in transaction receipts, embedded from go-ethereum's core.Receipt
- Tests show proper JSON marshaling without any dependency on the "root" field
- No other critical usages of receipt root exist in the codebase
The change aligns with Ethereum's post-Byzantium specification and correctly fixes Hardhat compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if "root" key is used elsewhere in the codebase
# Expected: No other critical usages of the "root" key in Ethereum RPC responses
# Search for "root" usage in JSON struct tags and RPC-related code
rg -g '*.go' '"root"'
# Search for any remaining references to transaction receipt root
rg -g '*.go' 'receipt.*root|root.*receipt'
Length of output: 616
Script:
#!/bin/bash
# Let's search more specifically for the transaction receipt structure and its usage
# Search for Receipt struct definition and its root field
ast-grep --pattern 'type Receipt struct {
$$$
root $_ $_
$$$
}'
# Search for any references to receipt root in comments or documentation
rg -g '*.go' -i 'receipt.*root|root.*receipt' -A 3 -B 3
# Search for any ethereum receipt related structs and their fields
rg -g '*.go' 'type.*Receipt.*struct' -A 10 -B 1
Length of output: 1692
Script:
#!/bin/bash
# Let's examine the go-ethereum core Receipt structure that we're embedding
# Search for any imports of go-ethereum/core to understand the embedded Receipt
rg -g '*.go' 'gethcore "github.com/ethereum/go-ethereum/core"' -A 5 -B 5
# Look for any other references to Receipt root in the codebase
rg -g '*.go' 'Receipt.*root|root.*Receipt' -A 3 -B 3
# Check for any tests related to transaction receipts
rg -g '*_test.go' 'TransactionReceipt|Receipt.*MarshalJSON' -A 5 -B 5
Length of output: 5638
CHANGELOG.md (1)
103-103
: LGTM! Changelog entry follows guidelines and accurately describes the change.
The changelog entry properly documents the removal of the deprecated root key from eth_getTransactionReceipt, which aligns with the PR objectives to fix Hardhat deployment issues. The entry follows the proper format with PR link and concise description.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2108 +/- ##
=======================================
Coverage 64.66% 64.66%
=======================================
Files 273 273
Lines 21598 21600 +2
=======================================
+ Hits 13966 13968 +2
Misses 6655 6655
Partials 977 977
|
Hardhat deploy failed on parsing
0x
root value ofeth_getTransactionReceipt
result.As soon as we have
status
value returned - that key is not required, see doc: https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_gettransactionreceiptSummary by CodeRabbit
eth_getTransactionReceipt
method to improve compatibility with Hardhat.CHANGELOG.md
with new entries for unreleased features and fixes.