refactor(evm): converted untyped event to typed and cleaned up #2053
Conversation
WalkthroughThe changes in this pull request involve extensive updates to the NibiruChain project, primarily focusing on enhancing the changelog format and content, improving event handling for Ethereum transactions, and refining testing practices. The changelog now follows the "Keep a Changelog" format and includes detailed sections for unreleased changes, features, improvements, and bug fixes. Additionally, several files have been updated to reflect new event types and attributes, with corresponding changes in tests to ensure accurate validation of Ethereum transaction handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Backend
participant EVM
participant EventEmitter
User->>Backend: Send Ethereum Transaction
Backend->>EVM: Process Transaction
EVM->>EventEmitter: Emit Ethereum Transaction Event
EventEmitter-->>Backend: Event Emitted
Backend-->>User: Transaction Receipt
Possibly related PRs
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2053 +/- ##
==========================================
- Coverage 65.45% 65.13% -0.32%
==========================================
Files 268 269 +1
Lines 16985 17007 +22
==========================================
- Hits 11117 11078 -39
- Misses 4941 5000 +59
- Partials 927 929 +2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (20)
app/evmante/evmante_emit_event.go (2)
44-51: LGTM! Consider clarifying the comment.The changes align well with the PR objectives. The event emission has been updated to use more specific names and attributes, which improves clarity and maintainability. The retention of an untyped event for Tendermint subscription compatibility is also in line with the PR goals.
Consider updating the comment to be more specific:
-// Untyped event: "message", used for tendermint subscription +// Untyped event: "pending_ethereum_tx", used for Tendermint TxSearch compatibilityThis change would more accurately reflect the event being emitted and its purpose as described in the PR objectives.
Remaining Untyped Events Found
Several untyped events are still present in the codebase:
x/inflation/keeper/hooks.gox/evm/keeper/msg_server.goapp/evmante/evmante_gas_consume.goPlease refactor these instances to use the newly implemented typed events to ensure consistency and adherence to PR objectives.
🔗 Analysis chain
Line range hint
1-55: Verify implementation of typed events mentioned in PR objectives.While this file correctly implements the untyped
pending_ethereum_txevent, the PR objectives mention several typed events that are not visible in this file. To ensure comprehensive implementation of the PR objectives, it would be beneficial to verify the implementation of the following typed events:
eth.evm.v1.EventEthereumTxeth.evm.v1.EventTxLogEventContractDeployedEventContractExecutedEventTransfereth.evm.v1.EventBlockBloomTo verify the implementation of these events, you can run the following script:
This script will help identify the locations where the typed events are implemented and highlight any remaining untyped events that may need attention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of typed events mentioned in PR objectives # Test: Search for the implementation of typed events echo "Searching for typed event implementations..." rg --type go -e 'EventEthereumTx' -e 'EventTxLog' -e 'EventContractDeployed' -e 'EventContractExecuted' -e 'EventTransfer' -e 'EventBlockBloom' # Test: Check for any remaining untyped events that should have been converted echo "Checking for any remaining untyped events..." rg --type go 'sdk.NewEvent\(' -g '!app/evmante/evmante_emit_event.go'Length of output: 17281
app/evmante/evmante_emit_event_test.go (2)
69-71: LGTM: Updated attribute key for transaction hashThe attribute key update from
AttributeKeyEthereumTxHashtoPendingEthereumTxEventAttrEthHashis consistent with the new event type and provides more specificity.Consider updating the error message for clarity:
-s.Require().True(ok, "tx hash attribute not found") +s.Require().True(ok, "pending Ethereum transaction hash attribute not found")This change would make the error message more specific and aligned with the new attribute key.
62-76: Summary of changes inevmante_emit_event_test.goThe updates in this file consistently rename the event type and attribute keys for pending Ethereum transactions:
- Event type changed to
PendingEthereumTxEvent- Transaction hash attribute key updated to
PendingEthereumTxEventAttrEthHash- Transaction index attribute key updated to
PendingEthereumTxEventTxAttrIndexThese changes align with the PR objectives of converting untyped events to typed events and cleaning up. They improve specificity and consistency in handling pending Ethereum transaction events.
To ensure full compatibility and prevent potential issues:
- Verify that these changes are consistently applied across the entire codebase.
- Update any relevant documentation or comments referencing the old event types and attribute keys.
- Consider adding a migration guide or deprecation notices if these changes might affect external integrations or APIs.
eth/rpc/events_parser_test.go (3)
16-172: LGTM: Well-structured table-driven tests with comprehensive scenarios.The
TestParseTxResultfunction is well-organized and covers a wide range of test cases, including happy paths and error scenarios. The use of a table-driven test approach enhances maintainability and readability.Consider adding a test case for an invalid
txIndexin thependingEthereumTxEventto ensure robust error handling.
184-201: LGTM: Well-implemented helper function for creating Ethereum transaction events.The
ethereumTxEventfunction correctly constructs the event usingsdk.TypedEventToEvent, which is appropriate for converting typed events toabci.Event.Consider improving the error handling to avoid panicking. Instead of panicking on error, you could return the error to the caller:
-func ethereumTxEvent(txHash string, txIndex int, gasUsed int, failed bool) abci.Event { +func ethereumTxEvent(txHash string, txIndex int, gasUsed int, failed bool) (abci.Event, error) { // ... (existing code) event, err := sdk.TypedEventToEvent( // ... (existing code) ) if err != nil { - panic(err) + return abci.Event{}, err } - return (abci.Event)(event) + return (abci.Event)(event), nil }This change would require updating the test cases to handle the potential error, but it would make the function more robust and easier to use in different contexts.
1-201: LGTM: Well-structured and comprehensive test file.This test file is well-organized, follows Go best practices for testing, and provides thorough coverage of various scenarios for the
ParseTxResultfunction. The use of helper functions and table-driven tests enhances readability and maintainability.Consider adding a brief comment at the beginning of the file explaining the purpose of these tests and their relationship to the
ParseTxResultfunction. This would provide helpful context for developers who may be new to this part of the codebase.x/evm/msg.go (1)
Line range hint
182-186: Approve Hash validation improvementThe update to the
ValidateBasicmethod enhances the validation process by comparingmsg.Hashwith the actual transaction hash. This change improves the integrity check of the transaction.Consider adding a brief comment explaining why this validation is important, e.g.:
// Validate that the stored hash matches the hash of the transaction // This ensures the integrity of the transaction dataeth/rpc/backend/blocks.go (1)
336-340: Improved event parsing and error handlingThe changes to the
BlockBloomfunction are a good improvement:
- Using
evm.EventBlockBloomFromABCIEventsimplifies the code and likely makes it more robust.- The error handling now allows the function to continue processing events if one fails to parse.
These changes should improve maintainability and reduce potential errors.
Consider adding a debug log when an event fails to parse:
blockBloomEvent, err := evm.EventBlockBloomFromABCIEvent(event) if err != nil { + b.logger.Debug("Failed to parse block bloom event", "error", err) continue }This would help with debugging if issues arise in the future.
CHANGELOG.md (4)
Line range hint
31-126: Consider improving consistency and clarity in the Unreleased section.The Unreleased section provides a comprehensive overview of recent changes. However, there are a few areas that could be improved:
Inconsistent use of periods at the end of entries. Some entries end with a period, while others don't. Consider standardizing this for better readability.
Some entries might benefit from additional context. For example, entries like "refactor(evm): embeds" or "refactor(evm): simplify ERC-20 keeper methods" could be expanded to explain the impact or reason for these changes.
There are numerous EVM-related changes. Consider grouping these entries under a dedicated "EVM Improvements" subsection for better organization and readability.
Line range hint
128-165: Consider consolidating duplicate dependency entries.The Dependencies section is comprehensive, but there are several duplicate entries for some dependencies. For example,
bufbuild/buf-setup-action,github.com/CosmWasm/wasmvm, andgoogle.golang.org/grpchave multiple entries. Consider consolidating these into single entries that show the full version progression, e.g.:- Bump `bufbuild/buf-setup-action` from 1.21.0 to 1.26.1, then to 1.43.0 (#1449, #1469, #1505, #1510, [#1537](https://github.com/NibiruChain/nibiru/pull/1537), [#1540](https://github.com/NibiruChain/nibiru/pull/1540), [#1544](https://github.com/NibiruChain/nibiru/pull/1544), [#2043](https://github.com/NibiruChain/nibiru/pull/2043), [#2057](https://github.com/NibiruChain/nibiru/pull/2057))This would make the section more concise and easier to read.
Line range hint
167-1037: Excellent historical record, consider standardizing version number format.The changelog provides a comprehensive history of changes across multiple versions, which is very helpful for users and developers. The inclusion of release links and commit ranges for some versions is a great practice. To further improve this section:
Consider standardizing the format for version numbers. For example, some are presented as
[v0.19.4]with a link, while others arev0.15.0without a link. Adopting a consistent format like[v0.15.0](link)for all versions would improve readability and navigation.If possible, include release links and commit ranges for all versions, not just some. This would make it easier for users to explore the changes in detail for any given version.
Line range hint
1-1037: Overall excellent changelog structure with minor improvement suggestions.The changelog follows the "Keep a Changelog" format and is well-organized in reverse chronological order. To further enhance its usability:
Consider adding a table of contents at the beginning of the document. This would allow users to quickly jump to specific versions or sections of interest.
Standardize the use of headers for change categories (e.g., "Features", "Bug Fixes", "API Breaking") across all versions. Some older versions use different header levels or naming conventions for these categories.
For very long changelogs like this one, consider splitting it into separate files for major versions (e.g., v0.x.x, v1.x.x) while maintaining a summary in the main CHANGELOG.md. This can help manage the file size and make it easier to navigate.
These changes would make the changelog even more user-friendly and easier to navigate, especially as the project continues to evolve.
x/evm/events.go (2)
22-22: Correct the comment to reference the correct typeThe comment on line 22 incorrectly references
TypeUrlEventEthereumTxinstead of the structEventEthereumTx.-// proto.MessageName(new(evm.TypeUrlEventEthereumTx)) +// proto.MessageName(new(evm.EventEthereumTx))
25-25: Typo in comment: 'attribuges' should be 'attributes'There's a typographical error in the comment on line 25.
-// Untyped events and attribuges +// Untyped events and attributeseth/rpc/events_parser.go (1)
94-94: Clarify error message for missing pending eventAt line 94, the error message
"EventEthereumTx without pending_ethereum_tx event"could be more informative. Including additional context can help with debugging.Consider revising the error message:
return nil, errors.New("EventEthereumTx encountered without corresponding pending_ethereum_tx event; this may indicate an inconsistency in event emission")eth/rpc/rpcapi/eth_api_test.go (2)
282-282: Avoid variable shadowing oferrAt lines 282 and 287, the variable
erris assigned within the loop:pendingEthTxEventHash, pendingEthTxEventIndex, err = evm.GetEthHashAndIndexFromPendingEthereumTxEvent(event)ethereumTx, err := evm.EventEthereumTxFromABCIEvent(event)Using
:=in line 287 whenerris already declared can lead to variable shadowing, which may cause unintended bugs if the outererrvariable is not updated as expected.Apply this diff to use assignment
=instead of:=and ensure consistency:if event.Type == evm.TypeUrlEventEthereumTx { - ethereumTx, err := evm.EventEthereumTxFromABCIEvent(event) + ethereumTx, err = evm.EventEthereumTxFromABCIEvent(event) s.Require().NoError(err) // ... }Also applies to: 287-287
Line range hint
546-547: Address the TODO: Implement direct JSON-RPC testing foreth_getTransactionByHashThe TODO comment indicates the need to test
eth_getTransactionByHashusing a JSON-RPC request directly to the endpoint:// TODO: Test eth_getTransactionByHash using a JSON-RPC request at the // endpoint directly.Implementing this test will enhance the coverage and ensure that the JSON-RPC endpoint behaves as expected.
Would you like assistance in writing this test case? I can help draft the code to perform a direct JSON-RPC request to the endpoint.
x/evm/keeper/msg_server.go (2)
700-705: Optimize log serialization in event emissionThe loop serializing logs to JSON strings could be optimized for performance, especially if there are many logs.
Consider preallocating the
txLogsslice and handling potential serialization errors efficiently.
739-740: Return nil explicitly on successful event emissionThe function
EmitEthereumTxEventsreturnsnilon success. Ensure this behavior is consistent and explicitly documented.Add a comment to clarify the return value:
// Return nil to indicate successful event emission return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/evm/events.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
- CHANGELOG.md (1 hunks)
- app/evmante/evmante_emit_event.go (1 hunks)
- app/evmante/evmante_emit_event_test.go (1 hunks)
- eth/indexer/evm_tx_indexer.go (1 hunks)
- eth/indexer/evm_tx_indexer_test.go (5 hunks)
- eth/rpc/backend/account_info_test.go (1 hunks)
- eth/rpc/backend/backend_suite_test.go (1 hunks)
- eth/rpc/backend/blocks.go (1 hunks)
- eth/rpc/backend/tx_info.go (2 hunks)
- eth/rpc/backend/utils.go (4 hunks)
- eth/rpc/events.go (0 hunks)
- eth/rpc/events_parser.go (1 hunks)
- eth/rpc/events_parser_test.go (1 hunks)
- eth/rpc/events_test.go (0 hunks)
- eth/rpc/rpcapi/eth_api_test.go (1 hunks)
- eth/rpc/rpcapi/eth_filters_api.go (1 hunks)
- eth/rpc/rpcapi/filter_utils.go (1 hunks)
- proto/eth/evm/v1/events.proto (0 hunks)
- x/evm/events.go (1 hunks)
- x/evm/keeper/msg_server.go (4 hunks)
- x/evm/msg.go (2 hunks)
- x/evm/msg_test.go (0 hunks)
💤 Files with no reviewable changes (4)
- eth/rpc/events.go
- eth/rpc/events_test.go
- proto/eth/evm/v1/events.proto
- x/evm/msg_test.go
✅ Files skipped from review due to trivial changes (2)
- eth/indexer/evm_tx_indexer.go
- eth/rpc/backend/account_info_test.go
🔇 Additional comments (23)
app/evmante/evmante_emit_event_test.go (2)
74-76: LGTM: Updated attribute key for transaction indexThe attribute key update from
AttributeKeyTxIndextoPendingEthereumTxEventTxAttrIndexis consistent with the new event type and provides more specificity.Consider updating the error message for clarity:
-s.Require().True(ok, "tx index attribute not found") +s.Require().True(ok, "pending Ethereum transaction index attribute not found")This change would make the error message more specific and aligned with the new attribute key.
Let's verify the resolution of the tx_index discrepancy mentioned in the PR objectives:
#!/bin/bash # Description: Verify the consistency of tx_index usage in ante handler and applied EVM events # Test: Search for occurrences of PendingEthereumTxEventTxAttrIndex in ante handler rg --type go "PendingEthereumTxEventTxAttrIndex" app/evmante # Test: Search for occurrences of tx_index in applied EVM events rg --type go "tx_index" x/evm
62-62: LGTM: Event type updated toPendingEthereumTxEventThis change aligns with the PR objectives of retaining an untyped event for compatibility with Tendermint TxSearch. The new event name accurately reflects its purpose for pending Ethereum transactions.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
: Event type
EventTypeEthereumTxhas been successfully updated toPendingEthereumTxEventacross the codebase. No remaining instances of the old event type were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of PendingEthereumTxEvent across the codebase # Test: Search for occurrences of PendingEthereumTxEvent rg --type go "PendingEthereumTxEvent" # Test: Check for any remaining occurrences of the old event type rg --type go "EventTypeEthereumTx"Length of output: 516
eth/rpc/events_parser_test.go (2)
3-14: LGTM: Import statements are appropriate and well-organized.The import statements include all necessary packages for testing, Ethereum operations, and project-specific modules. The use of
gethcommonas an alias for the Ethereum common package is a good practice for clarity.
174-182: LGTM: Well-implemented helper function for creating pending Ethereum transaction events.The
pendingEthereumTxEventfunction correctly constructs the event using constants from theevmpackage, ensuring consistency with the main codebase.eth/rpc/backend/backend_suite_test.go (1)
176-179: Improved error handling in WaitForReceipt functionThe changes to the
WaitForReceiptfunction enhance the error handling logic:
- Error checking is now performed at the beginning of the loop, which is a common Go pattern.
- The function returns
nil, nilimmediately if an error occurs, simplifying the control flow.These modifications improve code readability and maintain consistency with Go's idiomatic error handling practices. The changes align well with the PR's objective of refactoring and cleaning up EVM-related code.
eth/indexer/evm_tx_indexer_test.go (9)
51-53: LGTM: Improved transaction building and encodingThe changes in this segment enhance clarity and align with the PR objectives. The use of
validEVMTxandvalidEVMTxBzprovides more explicit naming, and thetx.BuildTxmethod suggests a more structured approach to transaction building.
59-60: LGTM: Simplified invalid transaction creationThe changes in this segment are consistent with the earlier modifications and simplify the invalid transaction creation process. The new variable name
invalidTxis more concise and clear.
70-74: LGTM: Improved test case clarityThe changes in this segment enhance the test case clarity. The new test case name "happy, only pending_ethereum_tx presents" is more descriptive and aligns well with the PR objectives. The use of
validEVMTxBzmaintains consistency with earlier changes.
79-85: LGTM: Enhanced event type and attributesThe changes in this segment directly address the PR objectives by converting untyped events to typed events. The new event type
evm.PendingEthereumTxEventand the updated attribute keysPendingEthereumTxEventAttrEthHashandPendingEthereumTxEventTxAttrIndexare more specific and descriptive, improving the overall clarity of the event structure.
92-113: LGTM: Comprehensive test case for combined eventsThis test case excellently demonstrates the coexistence of untyped and typed events, aligning perfectly with the PR objectives. The inclusion of both
PendingEthereumTxEvent(untyped) andTypeUrlEventEthereumTx(typed) events, along with the detailed attributes in the typed event, provides a thorough test scenario. This change enhances the test coverage and ensures that the new event structure is properly implemented and validated.
120-121: LGTM: Improved test case for gas limit scenarioThe changes in this segment enhance the clarity of the test case. The new name "happy: code 11, exceed block gas limit" is more descriptive and accurately reflects the scenario being tested. The use of
validEVMTxBzmaintains consistency with earlier changes in the file.
132-133: LGTM: Clearer test case for failed transactionThe changes in this segment improve the test case clarity. The new name "sad: failed eth tx" is concise and clearly indicates a negative scenario. The use of
validEVMTxBzmaintains consistency with earlier changes in the file.
144-145: LGTM: Enhanced clarity in negative test casesThe changes in these segments improve the clarity of the negative test cases. The new names "sad: invalid events" and "sad: not eth tx" are descriptive and clearly indicate the nature of these scenarios. The use of
validEVMTxBzandinvalidTxBzmaintains consistency with earlier changes and provides a clear distinction between valid and invalid transactions in the test setup.Also applies to: 155-156
Line range hint
1-203: Overall LGTM: Comprehensive refactoring of EVM event testsThis refactoring significantly improves the EVM transaction indexer tests, aligning perfectly with the PR objectives. Key improvements include:
- Clearer naming conventions for transactions and test cases.
- Introduction of both typed and untyped events, enhancing test coverage.
- More detailed event attributes, providing better transaction information.
- Consistent use of
validEVMTxBzandinvalidTxBzthroughout the test cases.- Improved clarity in test case descriptions, especially for negative scenarios.
These changes collectively enhance the test suite's clarity, consistency, and coverage, ensuring robust validation of the new event structure and indexing functionality.
x/evm/msg.go (1)
145-145: Verify impact of commented Type() methodThe
Type()method forMsgEthereumTxhas been commented out, and theTypeMsgEthereumTxconstant has been removed. This change might affect type-based routing or type checks elsewhere in the codebase.Please ensure that:
- All references to
TypeMsgEthereumTxhave been removed or updated.- Any code relying on the
Type()method ofMsgEthereumTxhas been adjusted accordingly.- The rationale for this change is documented in the code or commit message.
Run the following script to check for any remaining references:
✅ Verification successful
Type() method removal verified
No active references to
TypeMsgEthereumTxorMsgEthereumTx.Type()were found in the codebase, indicating that the commented outType()method and removed constant do not affect other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TypeMsgEthereumTx references rg --type go "TypeMsgEthereumTx" # Search for Type() method calls on MsgEthereumTx rg --type go "MsgEthereumTx.*\.Type\(\)"Length of output: 158
Script:
#!/bin/bash # Corrected search for TypeMsgEthereumTx references rg --type go "TypeMsgEthereumTx" # Corrected search for Type() method calls on MsgEthereumTx rg --type go "MsgEthereumTx\.Type\(\)"Length of output: 156
eth/rpc/rpcapi/eth_filters_api.go (1)
392-392: Approve change, but verify correctness and impactThe modification to use
TypeUrlEventEthereumTxaligns with the PR objective of converting to typed events. This change should improve the structure and type safety of the event system.Please run the following verification steps to ensure the change doesn't introduce any regressions:
- Confirm that
TypeUrlEventEthereumTxis the correct identifier for Ethereum transactions in the new typed event system.- Verify that this change doesn't break existing functionality, especially in terms of log filtering and subscription.
✅ Verification successful
Change Verified and Approved
The usage of
TypeUrlEventEthereumTxis consistent and correctly implemented across the codebase. The old identifierTypeMsgEthereumTxis no longer in active use, ensuring no regressions or inconsistencies in event handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of TypeUrlEventEthereumTx and its definition # Check if TypeUrlEventEthereumTx is defined and used consistently echo "Checking TypeUrlEventEthereumTx definition and usage:" rg --type go "TypeUrlEventEthereumTx" -C 3 # Verify if there are any remaining uses of the old TypeMsgEthereumTx echo "Checking for any remaining uses of TypeMsgEthereumTx:" rg --type go "TypeMsgEthereumTx" # Look for any potential issues or inconsistencies in event handling echo "Checking for potential issues in event handling:" rg --type go "ev\.Events\[" -C 5Length of output: 3239
x/evm/events.go (1)
81-106: FunctionGetEthHashAndIndexFromPendingEthereumTxEventimplementation is correctThe function correctly extracts
ethHashandtxIndexfrom the event attributes and handles error cases appropriately.eth/rpc/rpcapi/filter_utils.go (1)
118-128: Code changes enhance clarity and simplify event parsingThe refactoring of
ParseBloomFromEventsimproves the readability by:
- Directly assigning
bloomEventTypeusinggogoproto.MessageName(new(evm.EventBlockBloom)), eliminating unnecessary variables.- Using
evm.EventBlockBloomFromABCIEventfor event parsing, which aligns with the updated event handling methods.These changes streamline the function and make the codebase more maintainable.
eth/rpc/events_parser.go (1)
32-38: Efficient design ofParsedTxswith hash mappingThe use of a map
TxHashesto associate transaction hashes with their message indices inParsedTxsimproves retrieval efficiency and code clarity.eth/rpc/backend/tx_info.go (1)
309-310: Verify the use of pending transaction events in the queryThe query is constructed using
evm.PendingEthereumTxEventandevm.PendingEthereumTxEventAttrEthHashto find transactions by Ethereum hash. Please ensure that this change is intentional and aligns with the indexing strategy. If the intention is to query committed transactions rather than pending ones, you might need to useevm.EventEthereumTxandevm.AttributeKeyEthereumTxHashinstead.x/evm/keeper/msg_server.go (3)
684-685: Verify transaction hash assignmentWhen setting
eventEthereumTx.Hash, ensure that the hash corresponds correctly to the current transaction. This is crucial for transaction traceability and debugging.Confirm that
tmtypes.Tx(ctx.TxBytes()).Hash()produces the expected hash value. If necessary, adjust the hash calculation to align with the system's standards.
708-716: Ensure consistent event attributes for Tendermint subscriptionWhen emitting the "message" event, verify that all required attributes are correctly set and conform to the expected formats for Tendermint event subscriptions.
718-737: Comprehensive event emission based on transaction outcomeThe logic for emitting typed events (
EventContractDeployed,EventContractExecuted,EventTransfer) covers various transaction scenarios. Ensure all possible cases are handled, and consider if additional events are needed for other transaction types.
| typedEvent, ok := (typedProtoEvent).(*EventTxLog) | ||
| if !ok { | ||
| return nil, errors.Wrapf( | ||
| err, "failed to parse event of type %s", typeUrl) | ||
| } |
There was a problem hiding this comment.
Avoid wrapping a nil error in errors.Wrapf
In the functions EventTxLogFromABCIEvent, EventBlockBloomFromABCIEvent, and EventEthereumTxFromABCIEvent, the error variable err may be nil when the type assertion fails. Wrapping a nil error with errors.Wrapf can lead to misleading error messages.
For example, in EventTxLogFromABCIEvent:
typedEvent, ok := (typedProtoEvent).(*EventTxLog)
if !ok {
return nil, errors.Wrapf(
err, "failed to parse event of type %s", typeUrl)
}Since err is nil at this point, you should create a new error instead of wrapping err. Apply this change to all three functions:
if !ok {
- return nil, errors.Wrapf(
- err, "failed to parse event of type %s", typeUrl)
+ return nil, errors.Errorf(
+ "failed to parse event of type %s", typeUrl)
}Also applies to: 58-62, 73-77
| } | ||
| return bloom, err |
There was a problem hiding this comment.
Return an error when no Bloom event is found
Currently, if no event of type EventBlockBloom is found in the provided events, the function returns a zero bloom and a nil error. This can lead to unexpected behavior for callers expecting a valid bloom filter or an error indicating why it wasn't retrieved. To improve error handling, return a descriptive error when the bloom event is not found.
Apply this diff to return an appropriate error:
}
- return bloom, err
+ return bloom, errors.New("EventBlockBloom not found in events")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| return bloom, err | |
| } | |
| return bloom, errors.New("EventBlockBloom not found in events") |
| @@ -0,0 +1,169 @@ | |||
| // Copyright (c) 2023-2024 Nibi, Inc. | |||
There was a problem hiding this comment.
Correct the company name in the copyright notice
The copyright notice at line 1 mentions "Nibi, Inc.", but the correct company name is "Nibiru, Inc." Please update it to ensure proper attribution.
Apply this diff to correct the company name:
-// Copyright (c) 2023-2024 Nibi, Inc.
+// Copyright (c) 2023-2024 Nibiru, Inc.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Copyright (c) 2023-2024 Nibi, Inc. | |
| // Copyright (c) 2023-2024 Nibiru, Inc. |
eth/rpc/events_parser.go
Outdated
| func (p *ParsedTxs) AccumulativeGasUsed(msgIndex int) (result uint64) { | ||
| for i := 0; i <= msgIndex; i++ { | ||
| result += p.Txs[i].GasUsed | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Validate msgIndex to prevent potential out-of-bounds access
In the AccumulativeGasUsed method (lines 164-169), if msgIndex is negative or greater than or equal to the length of p.Txs, the loop may access invalid indices, leading to unexpected behavior.
Add validation for msgIndex:
func (p *ParsedTxs) AccumulativeGasUsed(msgIndex int) (result uint64) {
+ if msgIndex < 0 || msgIndex >= len(p.Txs) {
+ return 0
+ }
for i := 0; i <= msgIndex; i++ {
result += p.Txs[i].GasUsed
}
return result
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *ParsedTxs) AccumulativeGasUsed(msgIndex int) (result uint64) { | |
| for i := 0; i <= msgIndex; i++ { | |
| result += p.Txs[i].GasUsed | |
| } | |
| return result | |
| } | |
| func (p *ParsedTxs) AccumulativeGasUsed(msgIndex int) (result uint64) { | |
| if msgIndex < 0 || msgIndex >= len(p.Txs) { | |
| return 0 | |
| } | |
| for i := 0; i <= msgIndex; i++ { | |
| result += p.Txs[i].GasUsed | |
| } | |
| return result | |
| } |
eth/rpc/events_parser.go
Outdated
| parsedTxs.Txs[i].Failed = true | ||
|
|
||
| // replace gasUsed with gasLimit because that's what's actually deducted. | ||
| gasLimit := tx.GetMsgs()[i].(*evm.MsgEthereumTx).GetGas() |
There was a problem hiding this comment.
Handle potential panic due to type assertion without checking
At line 105, the code performs a type assertion without verifying the message type. If tx.GetMsgs()[i] is not of type *evm.MsgEthereumTx, this could cause a panic due to an invalid type assertion.
Consider adding a type check before the assertion:
+ msgEthereumTx, ok := tx.GetMsgs()[i].(*evm.MsgEthereumTx)
+ if !ok {
+ return nil, fmt.Errorf("unexpected message type at index %d", i)
+ }
- gasLimit := tx.GetMsgs()[i].(*evm.MsgEthereumTx).GetGas()
+ gasLimit := msgEthereumTx.GetGas()
parsedTxs.Txs[i].GasUsed = gasLimit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gasLimit := tx.GetMsgs()[i].(*evm.MsgEthereumTx).GetGas() | |
| msgEthereumTx, ok := tx.GetMsgs()[i].(*evm.MsgEthereumTx) | |
| if !ok { | |
| return nil, fmt.Errorf("unexpected message type at index %d", i) | |
| } | |
| gasLimit := msgEthereumTx.GetGas() |
| pendingEthTxEventHash := gethcommon.Hash{} | ||
| pendingEthTxEventIndex := int32(-1) | ||
| for _, event := range events { | ||
| if event.Type == "ethereum_tx" { | ||
| ethTxEvents = append(ethTxEvents, | ||
| sdk.Event{Type: event.Type, Attributes: event.Attributes}, | ||
| if event.Type == evm.PendingEthereumTxEvent { | ||
| pendingEthTxEventHash, pendingEthTxEventIndex, err = | ||
| evm.GetEthHashAndIndexFromPendingEthereumTxEvent(event) | ||
| s.Require().NoError(err) | ||
| } | ||
| if event.Type == evm.TypeUrlEventEthereumTx { | ||
| ethereumTx, err := evm.EventEthereumTxFromABCIEvent(event) | ||
| s.Require().NoError(err) | ||
| s.Require().Equal( | ||
| pendingEthTxEventHash.Hex(), | ||
| ethereumTx.EthHash, | ||
| "hash of pending ethereum tx event and ethereum tx event must be equal", | ||
| ) | ||
| s.Require().Equal( | ||
| fmt.Sprintf("%d", pendingEthTxEventIndex), | ||
| ethereumTx.Index, | ||
| "index of pending ethereum tx event and ethereum tx event must be equal", | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| eventsJson, _ := json.Marshal(events) | ||
| s.Require().Equal(len(ethTxEvents), 2, "events: ", eventsJson) | ||
| hash0, _ := ethTxEvents[0].GetAttribute(evm.AttributeKeyEthereumTxHash) | ||
| hash1, _ := ethTxEvents[1].GetAttribute(evm.AttributeKeyEthereumTxHash) | ||
| s.Require().Equal(hash0, hash1) | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure variables are assigned before use to prevent uninitialized comparisons
In the loop starting at line 280, if the event of type evm.PendingEthereumTxEvent is not encountered, the variables pendingEthTxEventHash and pendingEthTxEventIndex will retain their initial values (gethcommon.Hash{} and -1, respectively). This could lead to incorrect assertions when comparing these variables in the subsequent block for evm.TypeUrlEventEthereumTx.
Consider adding a check to verify that the evm.PendingEthereumTxEvent event was found before making assertions that rely on pendingEthTxEventHash and pendingEthTxEventIndex.
Apply this diff to ensure that the pending event has been found before performing comparisons:
+ pendingEventFound := false
for _, event := range events {
if event.Type == evm.PendingEthereumTxEvent {
pendingEthTxEventHash, pendingEthTxEventIndex, err =
evm.GetEthHashAndIndexFromPendingEthereumTxEvent(event)
s.Require().NoError(err)
+ pendingEventFound = true
}
if event.Type == evm.TypeUrlEventEthereumTx {
+ s.Require().True(pendingEventFound, "PendingEthereumTxEvent not found before TypeUrlEventEthereumTx")
ethereumTx, err := evm.EventEthereumTxFromABCIEvent(event)
s.Require().NoError(err)
s.Require().Equal(
pendingEthTxEventHash.Hex(),
ethereumTx.EthHash,
"hash of pending ethereum tx event and ethereum tx event must be equal",
)
s.Require().Equal(
fmt.Sprintf("%d", pendingEthTxEventIndex),
ethereumTx.Index,
"index of pending ethereum tx event and ethereum tx event must be equal",
)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pendingEthTxEventHash := gethcommon.Hash{} | |
| pendingEthTxEventIndex := int32(-1) | |
| for _, event := range events { | |
| if event.Type == "ethereum_tx" { | |
| ethTxEvents = append(ethTxEvents, | |
| sdk.Event{Type: event.Type, Attributes: event.Attributes}, | |
| if event.Type == evm.PendingEthereumTxEvent { | |
| pendingEthTxEventHash, pendingEthTxEventIndex, err = | |
| evm.GetEthHashAndIndexFromPendingEthereumTxEvent(event) | |
| s.Require().NoError(err) | |
| } | |
| if event.Type == evm.TypeUrlEventEthereumTx { | |
| ethereumTx, err := evm.EventEthereumTxFromABCIEvent(event) | |
| s.Require().NoError(err) | |
| s.Require().Equal( | |
| pendingEthTxEventHash.Hex(), | |
| ethereumTx.EthHash, | |
| "hash of pending ethereum tx event and ethereum tx event must be equal", | |
| ) | |
| s.Require().Equal( | |
| fmt.Sprintf("%d", pendingEthTxEventIndex), | |
| ethereumTx.Index, | |
| "index of pending ethereum tx event and ethereum tx event must be equal", | |
| ) | |
| } | |
| } | |
| eventsJson, _ := json.Marshal(events) | |
| s.Require().Equal(len(ethTxEvents), 2, "events: ", eventsJson) | |
| hash0, _ := ethTxEvents[0].GetAttribute(evm.AttributeKeyEthereumTxHash) | |
| hash1, _ := ethTxEvents[1].GetAttribute(evm.AttributeKeyEthereumTxHash) | |
| s.Require().Equal(hash0, hash1) | |
| } | |
| pendingEthTxEventHash := gethcommon.Hash{} | |
| pendingEthTxEventIndex := int32(-1) | |
| pendingEventFound := false | |
| for _, event := range events { | |
| if event.Type == evm.PendingEthereumTxEvent { | |
| pendingEthTxEventHash, pendingEthTxEventIndex, err = | |
| evm.GetEthHashAndIndexFromPendingEthereumTxEvent(event) | |
| s.Require().NoError(err) | |
| pendingEventFound = true | |
| } | |
| if event.Type == evm.TypeUrlEventEthereumTx { | |
| s.Require().True(pendingEventFound, "PendingEthereumTxEvent not found before TypeUrlEventEthereumTx") | |
| ethereumTx, err := evm.EventEthereumTxFromABCIEvent(event) | |
| s.Require().NoError(err) | |
| s.Require().Equal( | |
| pendingEthTxEventHash.Hex(), | |
| ethereumTx.EthHash, | |
| "hash of pending ethereum tx event and ethereum tx event must be equal", | |
| ) | |
| s.Require().Equal( | |
| fmt.Sprintf("%d", pendingEthTxEventIndex), | |
| ethereumTx.Index, | |
| "index of pending ethereum tx event and ethereum tx event must be equal", | |
| ) | |
| } | |
| } | |
| } |
| err = k.EmitEthereumTxEvents(ctx, tx, msg, evmResp, contractAddr) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to emit ethereum tx events") | ||
| } |
There was a problem hiding this comment.
Handle event emission errors without failing the transaction
Currently, if EmitEthereumTxEvents returns an error, the transaction is aborted. Emitting events is typically a non-critical operation and should not prevent successful transactions from completing.
Consider logging the error instead of returning it:
err = k.EmitEthereumTxEvents(ctx, tx, msg, evmResp, contractAddr)
if err != nil {
- return nil, errors.Wrap(err, "failed to emit ethereum tx events")
+ k.Logger(ctx).Error("failed to emit ethereum tx events", "error", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = k.EmitEthereumTxEvents(ctx, tx, msg, evmResp, contractAddr) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "failed to emit ethereum tx events") | |
| } | |
| err = k.EmitEthereumTxEvents(ctx, tx, msg, evmResp, contractAddr) | |
| if err != nil { | |
| k.Logger(ctx).Error("failed to emit ethereum tx events", "error", err) | |
| } |
x/evm/keeper/msg_server.go
Outdated
| if err = k.RefundGas(ctx, msg, msg.Gas()-evmResp.GasUsed, evmConfig.Params.EvmDenom); err != nil { | ||
| return nil, errors.Wrapf(err, "failed to refund gas leftover gas to sender %s", msg.From()) | ||
| } |
There was a problem hiding this comment.
Prevent negative gas refund calculation
The calculation msg.Gas() - evmResp.GasUsed could result in a negative value if evmResp.GasUsed exceeds msg.Gas(). This could lead to unintended behavior or panic due to negative gas values.
Consider ensuring the refund amount is non-negative:
+refundGas := msg.Gas() - evmResp.GasUsed
+if refundGas < 0 {
+ refundGas = 0
+}
if err = k.RefundGas(ctx, msg, refundGas, evmConfig.Params.EvmDenom); err != nil {
return nil, errors.Wrapf(err, "failed to refund leftover gas to sender %s", msg.From())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err = k.RefundGas(ctx, msg, msg.Gas()-evmResp.GasUsed, evmConfig.Params.EvmDenom); err != nil { | |
| return nil, errors.Wrapf(err, "failed to refund gas leftover gas to sender %s", msg.From()) | |
| } | |
| refundGas := msg.Gas() - evmResp.GasUsed | |
| if refundGas < 0 { | |
| refundGas = 0 | |
| } | |
| if err = k.RefundGas(ctx, msg, refundGas, evmConfig.Params.EvmDenom); err != nil { | |
| return nil, errors.Wrapf(err, "failed to refund leftover gas to sender %s", msg.From()) | |
| } |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Applied with respect of uint64
| if err != nil { | ||
| // when a transaction contains multiple msg, as long as one of the msg fails | ||
| // all gas will be deducted. so is not msg.Gas() | ||
| k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit()) | ||
| return nil, errors.Wrap(err, "failed to apply ethereum core message") | ||
| } | ||
|
|
||
| logs := evm.LogsToEthereum(res.Logs) | ||
| logs := evm.LogsToEthereum(evmResp.Logs) |
There was a problem hiding this comment.
Check for nil evmResp.Logs before conversion
Ensure that evmResp.Logs is not nil before calling evm.LogsToEthereum(evmResp.Logs) to avoid a nil pointer dereference.
Add a nil check:
+if evmResp.Logs != nil {
logs := evm.LogsToEthereum(evmResp.Logs)
+} else {
+ logs := []*gethcore.Log{}
+}Committable suggestion was skipped due to low confidence.
| evmResp, err := k.ApplyEvmMsg(tmpCtx, msg, nil, true, evmConfig, txConfig) | ||
| if err != nil { | ||
| // when a transaction contains multiple msg, as long as one of the msg fails | ||
| // all gas will be deducted. so is not msg.Gas() | ||
| k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit()) |
There was a problem hiding this comment.
Ensure evmResp is not nil before usage
After calling k.ApplyEvmMsg, ensure that evmResp is not nil to prevent potential nil pointer dereferences in subsequent code.
Consider adding a nil check:
evmResp, err := k.ApplyEvmMsg(tmpCtx, msg, nil, true, evmConfig, txConfig)
if err != nil {
// existing error handling
return nil, errors.Wrap(err, "failed to apply ethereum core message")
+} else if evmResp == nil {
+ return nil, errors.New("evmResp is nil after applying EVM message")
}Committable suggestion was skipped due to low confidence.
app/evmante/evmante_emit_event.go
Outdated
| // emit ethereum tx hash as an event so that it can be indexed by | ||
| // Tendermint for query purposes it's emitted in ante handler, so we can | ||
| // query failed transaction (out of block gas limit). | ||
| // Untyped event: "message", used for tendermint subscription |
There was a problem hiding this comment.
Can you expand on what this comment means?
There was a problem hiding this comment.
Wrote a better comment
eth/rpc/backend/account_info_test.go
Outdated
| @@ -195,7 +195,7 @@ func generateStorageKey(key gethcommon.Address, slot uint64) string { | |||
| // Concatenate key and slot | |||
| data := append(keyBytes, slotBytes...) | |||
|
|
|||
| // Hash the data using Keccak256 | |||
| // EthHash the data using Keccak256 | |||
There was a problem hiding this comment.
| // EthHash the data using Keccak256 | |
| // Compute the data hash using Keccak256 |
eth/rpc/backend/blocks.go
Outdated
| } | ||
| return gethcore.BytesToBloom(hexutils.HexToBytes(blockBloomEvent.Bloom)), nil | ||
| } | ||
| return gethcore.Bloom{}, errors.New("block bloom event is not found") |
There was a problem hiding this comment.
| return gethcore.Bloom{}, errors.New("block bloom event is not found") | |
| return gethcore.Bloom{}, errors.New(msgType + " not found in end block results") |
eth/rpc/backend/utils.go
Outdated
| @@ -212,7 +214,7 @@ func (b *Backend) retrieveEVMTxFeesFromBlock( | |||
| func AllTxLogsFromEvents(events []abci.Event) ([][]*gethcore.Log, error) { | |||
| allLogs := make([][]*gethcore.Log, 0, 4) | |||
| for _, event := range events { | |||
| if event.Type != evm.EventTypeTxLog { | |||
| if event.Type != proto.MessageName((*evm.EventTxLog)(nil)) { | |||
There was a problem hiding this comment.
| if event.Type != proto.MessageName((*evm.EventTxLog)(nil)) { | |
| if event.Type != proto.MessageName(new(evm.EventTxLog)) { |
Technically equivalent, but nil pointers look a bit concerning
eth/rpc/backend/utils.go
Outdated
| @@ -229,7 +231,7 @@ func AllTxLogsFromEvents(events []abci.Event) ([][]*gethcore.Log, error) { | |||
| // TxLogsFromEvents parses ethereum logs from cosmos events for specific msg index | |||
| func TxLogsFromEvents(events []abci.Event, msgIndex int) ([]*gethcore.Log, error) { | |||
| for _, event := range events { | |||
| if event.Type != evm.EventTypeTxLog { | |||
| if event.Type != proto.MessageName((*evm.EventTxLog)(nil)) { | |||
There was a problem hiding this comment.
| if event.Type != proto.MessageName((*evm.EventTxLog)(nil)) { | |
| if event.Type != proto.MessageName(new(evm.EventTxLog)) { |
Technically equivalent, but nil pointers look a bit concerning
eth/rpc/events.go
Outdated
x/evm/events.go
Outdated
| // Used in non-typed event "pending_ethereum_tx" | ||
| PendingEthereumTxEvent = "pending_ethereum_tx" | ||
| PendingEthereumTxEventAttrEthHash = "eth_hash" | ||
| PendingEthereumTxEventTxAttrIndex = "index" |
There was a problem hiding this comment.
| PendingEthereumTxEventTxAttrIndex = "index" | |
| PendingEthereumTxEventAttrIndex = "index" |
To follow the convention of the others
There was a problem hiding this comment.
Good catch! Fixed.
x/evm/msg.go
Outdated
| @@ -148,7 +142,7 @@ func (msg *MsgEthereumTx) FromEthereumTx(tx *gethcore.Transaction) error { | |||
| func (msg MsgEthereumTx) Route() string { return RouterKey } | |||
|
|
|||
| // Type returns the type value of an MsgEthereumTx. | |||
| func (msg MsgEthereumTx) Type() string { return TypeMsgEthereumTx } | |||
| //func (msg MsgEthereumTx) Type() string { return TypeMsgEthereumTx } | |||
There was a problem hiding this comment.
| //func (msg MsgEthereumTx) Type() string { return TypeMsgEthereumTx } | |
| func (msg MsgEthereumTx) Type() string { return TypeMsgEthereumTx } |
Looks like this was commented by mistake. If not please, disregard my suggestion and we can remove these lines.
There was a problem hiding this comment.
We can add a compile-time assertion here to make sure we don't break the interface. The code would be along the lines of:
import (
"github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx"
)
// ensure Msg interface compliance at compile time
var (
_ legacytx.LegacyMsg = &MsgEthereumTx
)This way the compiler will get upset if we remove this method.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
eth/indexer/evm_tx_indexer_test.go (3)
79-85: Updated event type and attributes for pending Ethereum transactions.The changes from
evm.EventTypeEthereumTxtoevm.PendingEthereumTxEventand the updated attribute keys align with the PR's objective of converting untyped events to typed events. This provides more detailed and specific information about Ethereum transactions.Consider adding a comment explaining the significance of the
PendingEthereumTxEventand how it differs from other transaction events. This would enhance code documentation and make it easier for other developers to understand the purpose of this specific event type.
92-113: Enhanced test case with both untyped and typed Ethereum transaction events.The updated test case now includes both the untyped
pending_ethereum_txand the new typedEventEthereumTx. This change aligns perfectly with the PR's objective of introducing typed events while maintaining compatibility with untyped events. The additional attributes in the typed event provide more comprehensive transaction details, which is beneficial for transaction receipt retrieval.To improve code readability, consider extracting the event attribute keys and values into constants. This would make it easier to maintain and update these values in the future, especially if they are used in multiple test cases.
Line range hint
132-156: Improved test case naming for various error scenarios.The test case names have been updated to provide more specific descriptions of the scenarios being tested:
- "sad: failed eth tx" for testing failed Ethereum transactions
- "sad: invalid events" for testing scenarios with invalid event data
- "sad: not eth tx" for testing the indexer's behavior with non-Ethereum transactions
These improvements in naming conventions enhance the overall readability and maintainability of the test suite, making it easier to understand the purpose of each test case.
Consider adding brief comments before each test case to explain the expected behavior or the specific aspect of the indexer being tested. This would further improve the documentation and make it easier for other developers to understand and maintain these tests in the future.
x/evm/msg.go (2)
Line range hint
181-185: Approve the updatedValidateBasic()method with a suggestionThe change improves the validation of the
Hashfield, ensuring consistency with the actual transaction data. This aligns well with the PR objectives.Consider updating the error message to be more specific:
- return errorsmod.Wrapf(errortypes.ErrInvalidRequest, "invalid tx hash %s, expected: %s", msg.Hash, txHash) + return errorsmod.Wrapf(errortypes.ErrInvalidRequest, "invalid EthHash %s, expected: %s", msg.Hash, txHash)This change would make the error message consistent with the updated comment and provide more clarity about the nature of the hash being validated.
Line range hint
193-209: Approve theGetSigners()method update with a suggestion for error handlingThe update ensures that the
Signmethod is called beforeGetSigners, which is crucial for correct operation. However, using a panic in production code might not be the best approach.Consider returning an error instead of panicking:
func (msg *MsgEthereumTx) GetSigners() ([]sdk.AccAddress, error) { data, err := UnpackTxData(msg.Data) if err != nil { return nil, fmt.Errorf("failed to unpack tx data: %w", err) } sender, err := msg.GetSender(data.GetChainID()) if err != nil { return nil, fmt.Errorf("failed to get sender: %w", err) } signer := sdk.AccAddress(sender.Bytes()) return []sdk.AccAddress{signer}, nil }This approach would allow callers to handle the error condition more gracefully. You'll need to update the interface and any calling code to handle the potential error.
x/evm/keeper/msg_server.go (2)
Line range hint
66-115: LGTM! Consider adding a nil check forevmResp.The changes improve consistency by using
evmRespthroughout the function. This refactoring enhances code readability and maintainability.However, consider adding a nil check for
evmRespafter theApplyEvmMsgcall to prevent potential nil pointer dereferences:if evmResp == nil { return nil, errors.New("ApplyEvmMsg returned nil response") }Also, the gas refund calculation (lines 110-115) could be extracted into a helper function for improved readability.
673-744: LGTM! Consider further decomposition and consistent error handling.The new
EmitEthereumTxEventsfunction is a good addition that centralizes event emission logic. It comprehensively covers different scenarios and emits both typed and untyped events.Suggestions for improvement:
- Consider decomposing this function into smaller, more focused functions for each event type. This would improve readability and maintainability.
- Implement consistent error handling across all event emissions. Currently, only some emissions check for errors.
- Replace magic strings with constants, e.g., replace
"message"on line 715 with a named constant.Example of decomposition:
func (k *Keeper) EmitEthereumTxEvents(ctx sdk.Context, tx *gethcore.Transaction, msg gethcore.Message, evmResp *evm.MsgEthereumTxResponse, contractAddr gethcommon.Address) error { if err := k.emitEventEthereumTx(ctx, tx, evmResp); err != nil { return err } if err := k.emitEventTxLog(ctx, evmResp); err != nil { return err } if err := k.emitMessageEvent(ctx, msg, tx); err != nil { return err } return k.emitTypedEvents(ctx, evmResp, msg, contractAddr) }This structure would make the function easier to read and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- app/evmante/evmante_emit_event.go (1 hunks)
- app/evmante/evmante_emit_event_test.go (1 hunks)
- eth/indexer/evm_tx_indexer_test.go (5 hunks)
- eth/rpc/backend/account_info_test.go (1 hunks)
- eth/rpc/backend/blocks.go (1 hunks)
- eth/rpc/backend/tx_info.go (2 hunks)
- eth/rpc/backend/utils.go (4 hunks)
- eth/rpc/events_parser.go (1 hunks)
- eth/rpc/events_parser_test.go (1 hunks)
- x/evm/events.go (1 hunks)
- x/evm/keeper/msg_server.go (4 hunks)
- x/evm/msg.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- app/evmante/evmante_emit_event.go
- app/evmante/evmante_emit_event_test.go
- eth/rpc/backend/account_info_test.go
- eth/rpc/backend/blocks.go
- eth/rpc/backend/tx_info.go
- eth/rpc/backend/utils.go
- eth/rpc/events_parser.go
- x/evm/events.go
🔇 Additional comments (8)
eth/indexer/evm_tx_indexer_test.go (5)
51-53: Improved variable naming for better clarity.The changes from
tmTxtovalidEVMTxandtxBztovalidEVMTxBzenhance code readability by using more descriptive variable names. This aligns well with the PR's objective of improving the structure of EVM events and transactions.
59-60: Consistent naming convention for invalid transaction.The change from
tmTxtoinvalidTxfor the invalid transaction maintains consistency with the previous naming improvements. This clear distinction between valid and invalid transactions enhances the test's readability and purpose.
70-74: Improved test case naming and event type specification.The test case name has been updated to "happy, only pending_ethereum_tx presents", which provides a clearer description of the test scenario. This change, along with the explicit mention of
pending_ethereum_tx, aligns well with the PR's objective of enhancing the structure of EVM events and improving clarity in the codebase.
120-121: Improved test case naming for gas limit scenario.The test case name has been updated to "happy: code 11, exceed block gas limit", which provides a clear and concise description of the scenario being tested. This improvement in naming conventions enhances the overall readability and maintainability of the test suite.
Line range hint
1-203: Overall assessment of changes in evm_tx_indexer_test.goThe changes made to this file significantly improve the clarity and effectiveness of the EVM transaction indexer tests. The updates align well with the PR objectives, particularly in distinguishing between typed and untyped events, and providing more comprehensive transaction details.
Key improvements include:
- More descriptive variable and test case naming
- Clear distinction between different event types (pending_ethereum_tx and EventEthereumTx)
- Enhanced test coverage for various scenarios, including error cases
These changes contribute to a more robust and maintainable test suite, which is crucial for ensuring the reliability of the EVM transaction indexer.
To further enhance the code quality, consider implementing the minor suggestions provided in the previous comments, such as adding explanatory comments and extracting constants for event attributes.
x/evm/msg.go (1)
144-144: Approve the change toType()method and verify its impactThe update to use
proto.MessageName()is a good refactoring choice, making the code more maintainable and consistent with protobuf conventions.Please run the following script to check for any other usages of the removed
TypeMsgEthereumTxconstant that might need updating:x/evm/keeper/msg_server.go (1)
125-140: LGTM! Good centralization of event emission logic.The changes continue to improve consistency by using
evmRespthroughout the function. The introduction ofEmitEthereumTxEventsis a good practice as it centralizes event emission logic, making the code more maintainable.The error handling for the new function call is properly implemented, addressing the concern raised in a previous review comment.
eth/rpc/events_parser_test.go (1)
186-188:⚠️ Potential issueConfirm the value of
EthTxFailedattribute inEventEthereumTx.Currently, the
EthTxFailedfield is set to the string"failed"when the transaction has failed. Please verify that this aligns with the expected value in theevm.EventEthereumTxstructure. IfEthTxFailedexpects a boolean string ("true"/"false") or another specific value, adjust the code accordingly to prevent any potential mismatches during event parsing.Run the following script to verify the expected values for
EthTxFailedin theEventEthereumTxdefinition:✅ Verification successful
The
EthTxFailedattribute inEventEthereumTxis correctly set to the string"failed", aligning with its intended use for capturing failure states or error messages. No adjustments are necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the expected values for `EthTxFailed` in `EventEthereumTx` definition. # Search for the definition of EventEthereumTx and display the EthTxFailed field. rg --type go 'type EventEthereumTx struct' -A 15 # Search for all assignments or usages of EthTxFailed to see expected values. rg --type go 'EthTxFailed' -A 2 -B 2Length of output: 3852
| { | ||
| MsgIndex: 0, | ||
| EthHash: txHashOne, | ||
| EthTxIndex: 0, | ||
| GasUsed: 0, | ||
| Failed: false, | ||
| }, | ||
| { | ||
| MsgIndex: 1, | ||
| EthHash: txHashTwo, | ||
| EthTxIndex: 1, | ||
| Failed: false, | ||
| }, |
There was a problem hiding this comment.
Missing GasUsed field in test case.
In the wantEthTxs slice of the test case "happy: two valid pending_ethereum_tx events", the second ParsedTx instance does not have the GasUsed field specified. This may lead to inconsistencies, especially since GasUsed is used in assertions later in the test. Please include the GasUsed field in all ParsedTx instances for consistency.
Apply this diff to add the missing GasUsed field:
{
MsgIndex: 1,
EthHash: txHashTwo,
EthTxIndex: 1,
+ GasUsed: 0,
Failed: false,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| MsgIndex: 0, | |
| EthHash: txHashOne, | |
| EthTxIndex: 0, | |
| GasUsed: 0, | |
| Failed: false, | |
| }, | |
| { | |
| MsgIndex: 1, | |
| EthHash: txHashTwo, | |
| EthTxIndex: 1, | |
| Failed: false, | |
| }, | |
| { | |
| MsgIndex: 0, | |
| EthHash: txHashOne, | |
| EthTxIndex: 0, | |
| GasUsed: 0, | |
| Failed: false, | |
| }, | |
| { | |
| MsgIndex: 1, | |
| EthHash: txHashTwo, | |
| EthTxIndex: 1, | |
| GasUsed: 0, | |
| Failed: false, | |
| }, |
Notes
Now, the EVM events are structured like this:
pending_ethereum_tx- untyped event, emitted within evm ante handler.Left untyped because it could be used by Tendermint TxSearch which cannot parse wrapper quotes in a typed event value.
eth.evm.v1.EventEthereumTx- typed event, contains more info about ethereum tx. Used for getting TxReceipt.message- untyped event with attrmodule (evm),senderandtx_type (legacy, accessList or DynamicFee)- left untyped because it's being used for event subscription (for eth it's used ineth_filterLogs)eth.evm.v1.EventTxLog- typed event, stores evm execution logs. Used in log queries.Additional typed events which are emitted depending on Tx action:
EventContractDeployed,EventContractExecutedorEventTransfer.eth.evm.v1.EventBlockBloom- typed event emitted in the end of the block and used for event filtering.TmTxIndexer vs EVM Indexer
One of the indexers must be turned on public nodes. With both turned off EVM Tx will not get a proper receipt and several queries will fail.
Fix tx_index
Fixed issue with
tx_indexdiscrepancy between ante handler event and apply evm event.Proto Break
Removed typed
EventMessageas we have to go with the untyped one. See above.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests