refactor(evm): remove double JSON escaping of EventEthereumTx logs
#2188
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of log emission and event handling within the Ethereum Virtual Machine (EVM) context for the Nibiru project. The changes primarily focus on streamlining log parsing, event type checking, and error handling across multiple components. Key modifications include updating event structures, removing redundant log-related functions, and simplifying the way transaction logs are emitted and processed. Changes
Sequence DiagramsequenceDiagram
participant Keeper
participant Context
participant EventManager
participant EVMResponse
Keeper->>EVMResponse: Execute Contract
EVMResponse-->>Keeper: Return Logs
Keeper->>Context: Get Event Manager
Context->>EventManager: Emit Typed Event
EventManager->>EventManager: Create EventTxLog
EventManager-->>Context: Event Emitted
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
k-yang
added
P: Nice to have / Low prio
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/evm/logs.go (1)
70-76: Return struct by value
ReturningLogby value can reduce heap allocations. However, consider handling a potential nilgethcore.Logto avoid unexpected panics.Here's a possible check:
func NewLogFromEth(log *gethcore.Log) Log { + if log == nil { + return Log{} + } topics := make([]string, len(log.Topics)) ... }x/evm/keeper/call_contract.go (1)
90-93: Directly emitting typed event
Emitting typed events improves traceability for clients. The error wrapping is fine; consider including more context (e.g., the transaction hash) in the error message for easier debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
x/evm/events.pb.gois excluded by!**/*.pb.gox/evm/evm.pb.gois excluded by!**/*.pb.gox/evm/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
CHANGELOG.md(1 hunks)eth/rpc/backend/tx_logs_test.go(1 hunks)eth/rpc/backend/utils.go(3 hunks)eth/rpc/events_parser.go(3 hunks)eth/rpc/events_parser_test.go(1 hunks)proto/eth/evm/v1/events.proto(2 hunks)proto/eth/evm/v1/evm.proto(1 hunks)proto/eth/evm/v1/tx.proto(10 hunks)x/evm/events.go(1 hunks)x/evm/keeper/call_contract.go(1 hunks)x/evm/keeper/msg_server.go(3 hunks)x/evm/logs.go(3 hunks)x/evm/logs_test.go(0 hunks)x/evm/msg.go(0 hunks)x/evm/msg_test.go(0 hunks)
💤 Files with no reviewable changes (3)
- x/evm/msg_test.go
- x/evm/msg.go
- x/evm/logs_test.go
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- proto/eth/evm/v1/tx.proto
🧰 Additional context used
🪛 GitHub Check: break-check
proto/eth/evm/v1/events.proto
[failure] 33-33:
Field "1" with name "logs" on message "EventTxLog" changed option "json_name" from "txLogs" to "logs".
[failure] 33-33:
Field "1" with name "logs" on message "EventTxLog" changed type from "string" to "message".
[failure] 33-33:
Field "1" on message "EventTxLog" changed name from "tx_logs" to "logs".
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: integration-tests
- GitHub Check: e2e-evm
🔇 Additional comments (17)
x/evm/logs.go (2)
51-55: Use of slice-of-value for logs
Switching the return type to a slice of values (instead of pointer slices) can reduce pointer overhead and improve clarity.
Line range hint
61-65: Consistent parameter type
Accepting[]Loghere is consistent with the updated return type fromNewLogsFromEth. This helps maintain a simpler data flow without pointer indirection.x/evm/events.go (3)
40-45: Inline usage of type URL
Directly referencingTypeUrlEventTxLogin the wrapped error provides clarity and consistency in the error messages.
54-59: Consistent use of block bloom type constant
UsingTypeUrlEventBlockBloomrather than a local variable ensures errors accurately reflect the correct type URL.
68-73: Improved error context for EthereumTx events
ReferencingTypeUrlEventEthereumTxdirectly makes logging and debugging more straightforward.eth/rpc/events_parser_test.go (1)
185-194: LGTM! Improved error handling withVmError.The change from
EthTxFailedtoVmErroris a good improvement that provides more accurate error representation for failed transactions.eth/rpc/events_parser.go (1)
52-52: LGTM! Improved event type checking and error handling.The changes improve code clarity by:
- Using direct event type references instead of constants
- Updating failure detection to use
VmErrorAlso applies to: 65-65, 83-83
eth/rpc/backend/utils.go (2)
Line range hint
214-230: LGTM! Simplified log parsing logic.The changes improve the code by:
- Using direct event type references
- Simplifying log parsing with direct calls to
evm.EventTxLogFromABCIEvent- Providing clearer error messages
242-262: LGTM! Streamlined block logs retrieval.The changes improve the code by:
- Removing redundant function calls
- Directly processing events to extract transaction logs
- Using consistent error handling
eth/rpc/backend/tx_logs_test.go (1)
243-246: LGTM! Updated test assertions for new log parsing.The changes correctly adapt the test assertions to use the new log parsing approach with
evm.EventTxLogFromABCIEvent.x/evm/keeper/msg_server.go (3)
97-100: LGTM! Streamlined log emission.The change simplifies log emission by directly using typed events, which aligns with the PR objective to remove double JSON escaping.
687-687: LGTM! Updated event field assignment.The change correctly assigns VM error to the new
vm_errorfield, maintaining consistency with the event structure changes.
735-738: LGTM! Block bloom filter update.The change correctly updates the block bloom filter and log size based on the transaction logs.
proto/eth/evm/v1/events.proto (3)
7-7: LGTM! Required import for Log type.The import is necessary for using the Log type in EventTxLog message.
33-33:⚠️ Potential issueVerify impact of log field changes.
The change from
tx_logs(string) tologs(Log type) is a breaking change. This change:
- Removes double JSON escaping by using a structured type
- Changes the field name from
tx_logstologs- Changes the type from string to Log message
Ensure that downstream clients are prepared to handle these changes.
Run the following script to find potential usages of the old field:
🧰 Tools
🪛 GitHub Check: break-check
[failure] 33-33:
Field "1" with name "logs" on message "EventTxLog" changed option "json_name" from "txLogs" to "logs".
[failure] 33-33:
Field "1" with name "logs" on message "EventTxLog" changed type from "string" to "message".
[failure] 33-33:
Field "1" on message "EventTxLog" changed name from "tx_logs" to "logs".
25-27:⚠️ Potential issueVerify impact on downstream clients.
The replacement of
eth_tx_failedwithvm_erroris a breaking change. Ensure that downstream clients (RouteScan and internal indexer) are notified and prepared to handle this change.Run the following script to find potential usages of the removed field:
✅ Verification successful
Breaking change concern resolved - change is backward compatible
The replacement of
eth_tx_failedwithvm_errorhas been implemented safely:
- Field 7 is properly reserved preventing future conflicts
- New
vm_errorfield uses a different number (8)- No direct dependencies on the removed field found in the codebase
- Error handling code has been properly updated to use the new field
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential usages of the removed eth_tx_failed field. # Search for eth_tx_failed in the codebase rg -i "eth_tx_failed" # Search for EventEthereumTx field access patterns ast-grep --pattern 'EventEthereumTx.$_'Length of output: 124
Script:
#!/bin/bash # Search for generated protobuf files and their content fd -e pb.go -e pb.ts -e pb.js -e _pb2.py # Search for any file containing "events" and "ethereum" to catch potential client implementations rg -i -l "events.*ethereum|ethereum.*events" # Search for potential imports of the events proto rg -i "import.*events.*proto|from.*events.*proto"Length of output: 1321
Script:
#!/bin/bash # Check content of key files echo "=== events.pb.go ===" cat x/evm/events.pb.go echo -e "\n=== msg_server.go ===" cat x/evm/keeper/msg_server.go | grep -A 10 -B 10 "Event" echo -e "\n=== events_parser_test.go ===" cat eth/rpc/events_parser_test.go | grep -A 10 -B 10 "Event" echo -e "\n=== eth_filters_api.go ===" cat eth/rpc/rpcapi/eth_filters_api.go | grep -A 10 -B 10 "Event"Length of output: 67586
proto/eth/evm/v1/evm.proto (1)
112-115: LGTM! Comment formatting.The changes are purely cosmetic, improving the readability of the comment without affecting functionality.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2188 +/- ##
==========================================
- Coverage 65.01% 64.77% -0.24%
==========================================
Files 277 277
Lines 22341 22276 -65
==========================================
- Hits 14524 14430 -94
- Misses 6828 6861 +33
+ Partials 989 985 -4
|
Purpose / Abstract
Mainly gets rid of double JSON escaping for
EventEthereumTx.LogsinBlockResults, resulting in easier parsing for downstream clients.It turns
{ "type": "eth.evm.v1.EventTxLog", "attributes": [ { "key": "tx_logs", "value": "[\"{\\\"address\\\":\\\"0x7D4B7B8CA7E1a24928Bb96D59249c7a5bd1DfBe6\\\",\\\"topics\\\":[\\\"0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef\\\",\\\"0x0000000000000000000000000000000000000000000000000000000000000000\\\",\\\"0x000000000000000000000000faf227dad0b91c2debd41dae71c959ea4f95f8f8\\\"],\\\"data\\\":\\\"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAPQkA=\\\",\\\"blockNumber\\\":105284,\\\"transactionHash\\\":\\\"0x0000000000000000000000000000000000000000000000000000000000000000\\\",\\\"transactionIndex\\\":0,\\\"blockHash\\\":\\\"0x50729736e2b9a83677f8aa418a52c6bfe0d181f850db1b92584eae71c043e97f\\\",\\\"logIndex\\\":0}\"]", "index": true } ] }into:
{ "type": "eth.evm.v1.EventTxLog", "attributes": [ { "key": "logs", "value": "[{\"address\":\"0xe5F54D19AA5c3c16ba70bC1E5112Fe37F1764134\",\"topics\":[\"0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef\",\"0x000000000000000000000000faf227dad0b91c2debd41dae71c959ea4f95f8f8\",\"0x000000000000000000000000603871c2ddd41c26ee77495e2e31e6de7f9957e0\"],\"data\":\"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAE=\",\"block_number\":\"634\",\"tx_hash\":\"0x5999f8f99366bf49fff0921f46006b0a71b3341a4cbcfa7fa3b045a9eebd2654\",\"tx_index\":\"0\",\"block_hash\":\"0xb04bca6e415c30667a3ab4f3e2fca03bb0fd5c83241481e294a6ea946b9f098e\",\"index\":\"0\",\"removed\":false}]", "index": true } ] }Also removes a bunch of redundant/unused code, and renames some fields for clarity.
BREAKING CHANGE: Note that this will break downstream clients that currently read logs. Since we're still pre-mainnet launch, it's better to do this change now than when we're live. The two main clients I can think of on testnet are RouteScan and our internal indexer.