refactor(eth-rpc): Delete unused code and improve logging in the eth and debug namespaces #2030
Conversation
WalkthroughThe changes involve a comprehensive refactor of the Ethereum-related codebase, focusing on enhancing code quality, improving logging, and updating testing strategies. Key modifications include the introduction of new functions for event subscription and bloom filter handling, renaming and restructuring existing functions for clarity, and the removal of obsolete test files. Additionally, several files were updated to improve documentation and error handling, while unused code was eliminated to streamline the codebase. Changes
Poem
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 (
|
Unique-Divine
changed the title
Unique-Divine
changed the title
Unique-Divine
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2030 +/- ##
==========================================
+ Coverage 66.26% 66.42% +0.16%
==========================================
Files 265 266 +1
Lines 16749 16819 +70
==========================================
+ Hits 11098 11172 +74
+ Misses 4811 4802 -9
- Partials 840 845 +5
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (8)
app/evmante/evmante_emit_event.go (1)
57-59: Address the duplicate event emission and prioritize the suggested improvements.The comment raises a valid concern about the current implementation where each event is emitted twice. This duplication could lead to inefficiencies or inconsistencies in event handling.
I agree with the suggested improvements:
- Migrating to typed events would provide a more structured and maintainable approach to event handling.
- Aligning with the EVM indexer is crucial for ensuring consistency and compatibility between the event emission and indexing mechanisms.
Please prioritize these improvements in the roadmap to enhance the overall event handling mechanism.
Additionally, consider including the commented-out attribute
"emitted_from"to provide more context about the event emission source. This information could be valuable for debugging and tracing purposes.eth/rpc/rpcapi/event_subscriber_test.go (1)
31-69: LGTM! Consider adding more test cases.The
TestEventSubscriberfunction is well-structured and covers the basic functionality ofEventSubscriber. It checks the creation and reuse of topic channels when installing subscriptions.To further improve the test coverage, consider adding test cases for:
- Uninstalling subscriptions
- Actual event handling and delivery to the subscriptions
eth/rpc/rpcapi/filter_utils.go (1)
118-140: LGTM with a minor optimization suggestion!The
ParseBloomFromEventsfunction looks good and correctly handles parsing the bloom filter from the provided events. It appropriately checks for the desired event type, parses it into a typed proto event, and converts the bloom data from hexadecimal format. The error handling is also well-implemented, providing informative error messages usingerrors.Wrapf.Just a minor optimization suggestion:
Consider returning early when the desired event is found and successfully parsed. This can be achieved by moving the
return bloom, errstatement inside the loop, right after the successful parsing and conversion of the bloom data. This way, the function can exit early without unnecessarily iterating through the remaining events.Apply this diff to optimize the function:
func ParseBloomFromEvents(events []abci.Event) (bloom gethcore.Bloom, err error) { bloomEvent := new(evm.EventBlockBloom) bloomEventType := gogoproto.MessageName(bloomEvent) for _, event := range events { if event.Type != bloomEventType { continue } typedProtoEvent, err := sdk.ParseTypedEvent(event) if err != nil { return bloom, errors.Wrapf( err, "failed to parse event of type %s", bloomEventType) } bloomEvent, ok := (typedProtoEvent).(*evm.EventBlockBloom) if !ok { return bloom, errors.Wrapf( err, "failed to parse event of type %s", bloomEventType) } - return eth.BloomFromHex(bloomEvent.Bloom) + bloom, err = eth.BloomFromHex(bloomEvent.Bloom) + if err != nil { + return bloom, err + } + return bloom, nil } - return bloom, err + return bloom, nil }eth/rpc/rpcapi/eth_api_test.go (4)
Line range hint
138-146: Address the TODO comment.The
Test_StorageAtmethod changes are part of the test suite refactor to associate the test with theNodeSuite. The test logic, which verifies the storage returned by theeth_getStorageAtRPC method, remains the same. However, there is a TODO comment indicating that more checks need to be added to the test.Do you want me to open a GitHub issue to track the task of adding more checks to this test?
Line range hint
148-157: Address the TODO comment.The
Test_PendingStorageAtmethod changes are part of the test suite refactor to associate the test with theNodeSuite. The test logic, which verifies the pending storage returned by theeth_getStorageAtRPC method with thependingtag, remains the same. However, there is a TODO comment indicating that more checks need to be added to the test.Do you want me to open a GitHub issue to track the task of adding more checks to this test?
Line range hint
159-166: Address the TODO comment.The
Test_CodeAtmethod changes are part of the test suite refactor to associate the test with theNodeSuite. The test logic, which verifies the code returned by theeth_getCodeRPC method, remains the same. However, there is a TODO comment indicating that more checks need to be added to the test.Do you want me to open a GitHub issue to track the task of adding more checks to this test?
Line range hint
168-175: Address the TODO comment.The
Test_PendingCodeAtmethod changes are part of the test suite refactor to associate the test with theNodeSuite. The test logic, which verifies the pending code returned by theeth_getCodeRPC method, remains the same. However, there is a TODO comment indicating that more checks need to be added to the test.Do you want me to open a GitHub issue to track the task of adding more checks to this test?
eth/rpc/rpcapi/eth_filters_api.go (1)
Line range hint
297-355: Improve test coverage and good job on addressing the TODO item!
- The debug log statement enhances traceability of operations, which is good. However, the static analysis tool has flagged some lines in this segment as not being covered by tests. Please add test cases to cover these lines and ensure they're being exercised:
- Lines 300, 340-343, 345
Do you want me to generate test cases to cover these lines or open a GitHub issue to track this task?
- The improvement to the handling of bloom filters by parsing bloom data from end block events is a positive change that addresses a previously marked TODO item. Good job on that!
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (30)
- CHANGELOG.md (1 hunks)
- app/evmante/evmante_emit_event.go (1 hunks)
- e2e/evm/test/basic_queries.test.ts (0 hunks)
- e2e/evm/test/debug_queries.test.ts (1 hunks)
- e2e/evm/test/eth_queries.test.ts (1 hunks)
- eth/indexer/kv_indexer.go (1 hunks)
- eth/rpc/backend/blocks.go (2 hunks)
- eth/rpc/backend/call_tx.go (2 hunks)
- eth/rpc/backend/filters.go (1 hunks)
- eth/rpc/backend/tx_info.go (4 hunks)
- eth/rpc/rpc.go (1 hunks)
- eth/rpc/rpcapi/apis.go (1 hunks)
- eth/rpc/rpcapi/eth_api_test.go (16 hunks)
- eth/rpc/rpcapi/eth_filters_api.go (19 hunks)
- eth/rpc/rpcapi/event_subscriber.go (1 hunks)
- eth/rpc/rpcapi/event_subscriber_test.go (1 hunks)
- eth/rpc/rpcapi/filter_utils.go (2 hunks)
- eth/rpc/rpcapi/filters.go (5 hunks)
- eth/rpc/rpcapi/filtersapi/filter_system.go (0 hunks)
- eth/rpc/rpcapi/filtersapi/filter_system_test.go (0 hunks)
- eth/rpc/rpcapi/net_api_test.go (1 hunks)
- eth/rpc/rpcapi/subscription.go (3 hunks)
- eth/rpc/rpcapi/websockets.go (6 hunks)
- eth/stringify.go (1 hunks)
- eth/stringify_test.go (1 hunks)
- x/common/address.go (1 hunks)
- x/evm/cli/query.go (0 hunks)
- x/evm/evmtest/tx.go (3 hunks)
- x/evm/keeper/hooks.go (1 hunks)
- x/evm/keeper/msg_server.go (1 hunks)
Files not reviewed due to no reviewable changes (4)
- e2e/evm/test/basic_queries.test.ts
- eth/rpc/rpcapi/filtersapi/filter_system.go
- eth/rpc/rpcapi/filtersapi/filter_system_test.go
- x/evm/cli/query.go
Files skipped from review due to trivial changes (4)
- eth/rpc/rpcapi/apis.go
- eth/rpc/rpcapi/filters.go
- x/common/address.go
- x/evm/keeper/msg_server.go
Additional context used
GitHub Check: codecov/patch
eth/rpc/rpc.go
[warning] 268-274: eth/rpc/rpc.go#L268-L274
Added lines #L268 - L274 were not covered by tests
[warning] 276-276: eth/rpc/rpc.go#L276
Added line #L276 was not covered by testseth/rpc/rpcapi/event_subscriber.go
[warning] 96-97: eth/rpc/rpcapi/event_subscriber.go#L96-L97
Added lines #L96 - L97 were not covered by tests
[warning] 102-106: eth/rpc/rpcapi/event_subscriber.go#L102-L106
Added lines #L102 - L106 were not covered by tests
[warning] 108-109: eth/rpc/rpcapi/event_subscriber.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 111-117: eth/rpc/rpcapi/event_subscriber.go#L111-L117
Added lines #L111 - L117 were not covered by tests
[warning] 120-121: eth/rpc/rpcapi/event_subscriber.go#L120-L121
Added lines #L120 - L121 were not covered by tests
[warning] 125-133: eth/rpc/rpcapi/event_subscriber.go#L125-L133
Added lines #L125 - L133 were not covered by tests
[warning] 136-138: eth/rpc/rpcapi/event_subscriber.go#L136-L138
Added lines #L136 - L138 were not covered by tests
[warning] 142-143: eth/rpc/rpcapi/event_subscriber.go#L142-L143
Added lines #L142 - L143 were not covered by tests
[warning] 145-147: eth/rpc/rpcapi/event_subscriber.go#L145-L147
Added lines #L145 - L147 were not covered by tests
[warning] 150-151: eth/rpc/rpcapi/event_subscriber.go#L150-L151
Added lines #L150 - L151 were not covered by tests
[warning] 157-162: eth/rpc/rpcapi/event_subscriber.go#L157-L162
Added lines #L157 - L162 were not covered by testseth/rpc/backend/tx_info.go
[warning] 60-60: eth/rpc/backend/tx_info.go#L60
Added line #L60 was not covered by tests
[warning] 357-357: eth/rpc/backend/tx_info.go#L357
Added line #L357 was not covered by testseth/rpc/rpcapi/eth_filters_api.go
[warning] 178-178: eth/rpc/rpcapi/eth_filters_api.go#L178
Added line #L178 was not covered by tests
[warning] 300-300: eth/rpc/rpcapi/eth_filters_api.go#L300
Added line #L300 was not covered by tests
[warning] 340-343: eth/rpc/rpcapi/eth_filters_api.go#L340-L343
Added lines #L340 - L343 were not covered by tests
[warning] 345-345: eth/rpc/rpcapi/eth_filters_api.go#L345
Added line #L345 was not covered by tests
[warning] 365-366: eth/rpc/rpcapi/eth_filters_api.go#L365-L366
Added lines #L365 - L366 were not covered by tests
[warning] 444-444: eth/rpc/rpcapi/eth_filters_api.go#L444
Added line #L444 was not covered by tests
[warning] 503-503: eth/rpc/rpcapi/eth_filters_api.go#L503
Added line #L503 was not covered by tests
[warning] 520-520: eth/rpc/rpcapi/eth_filters_api.go#L520
Added line #L520 was not covered by tests
[warning] 524-524: eth/rpc/rpcapi/eth_filters_api.go#L524
Added line #L524 was not covered by tests
[warning] 536-536: eth/rpc/rpcapi/eth_filters_api.go#L536
Added line #L536 was not covered by tests
[warning] 539-543: eth/rpc/rpcapi/eth_filters_api.go#L539-L543
Added lines #L539 - L543 were not covered by tests
[warning] 578-578: eth/rpc/rpcapi/eth_filters_api.go#L578
Added line #L578 was not covered by tests
[warning] 594-594: eth/rpc/rpcapi/eth_filters_api.go#L594
Added line #L594 was not covered by tests
[warning] 606-606: eth/rpc/rpcapi/eth_filters_api.go#L606
Added line #L606 was not covered by tests
[warning] 625-625: eth/rpc/rpcapi/eth_filters_api.go#L625
Added line #L625 was not covered by tests
Additional comments not posted (46)
eth/rpc/rpcapi/net_api_test.go (1)
Line range hint
7-13: LGTM!The renaming of the test suite from
TestSuitetoNodeSuitesuggests a potential restructuring of the test organization, possibly indicating a more focused or specialized context for the tests related to network functionality in the Ethereum RPC API. The change in the suite name may imply a shift in the scope or categorization of the tests, although the core functionality of the test itself remains unchanged.eth/stringify_test.go (1)
10-23: LGTM!The test function is well-structured and covers important test cases. The use of
gethcore.BytesToBloomto generate test data, and the use ofs.NoErrorands.Equalfassertions to validate the results, are good practices. The for loop to iterate over the test cases is a common pattern in Go test functions.x/evm/keeper/hooks.go (1)
23-23: LGTM! The change to useeth.BloomToHexis an improvement.The change to use the
eth.BloomToHexfunction instead of the genericeth.BytesToHexfunction for converting the bloom filter to a hex string is a good one. Theeth.BloomToHexfunction is likely a specialized function for converting bloom filters to hex strings, which may improve the accuracy or efficiency of the bloom data representation in emitted events.eth/stringify.go (4)
14-16: LGTM!The function is well-documented, and the implementation is accurate. The use of
fmt.Sprintfwith the%Xverb is the correct way to convert a byte slice to a capitalized hex-encoded string.
19-21: LGTM!The function is correctly implemented by using the
Hexmethod of thegethcommon.Hashtype, which returns the hex-encoded string representation of the hash.
29-35: LGTM!The function is correctly implemented and follows the best practices:
- The function uses the
hex.DecodeStringfunction to convert the hex-encoded string to a byte slice.- The function returns an error if the hex-encoded string is invalid.
- The function uses the
gethcore.BytesToBloomfunction to convert the byte slice to a bloom filter.
24-26: Verify theBytesToHexfunction.The function is correctly implemented, but the
BytesToHexfunction is not defined in this file. Please ensure that theBytesToHexfunction is defined in another file and imported correctly.Run the following script to verify the
BytesToHexfunction:Verification successful
Verification successful for
BytesToHexfunction usage.The
BytesToHexfunction is defined ineth/state_encoder.go, confirming that theBloomToHexfunction ineth/stringify.gois correctly implemented. No issues are found with the function call.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `BytesToHex` function is defined in the codebase. # Test: Search for the function definition. Expect: At least one occurrence. rg --type go $'func BytesToHex'Length of output: 89
eth/rpc/backend/filters.go (1)
33-39: LGTM!The changes to the
BloomStatusfunction improve its documentation and readability without altering its behavior. Naming the return values in the function signature makes it clearer what the function returns, and the enhanced comments explain the meaning of each return value.Consider revisiting the hardcoded return values in the future to ensure they align with the actual state of the bloom filter on the server side. But for now, these changes are good to go.
eth/rpc/rpcapi/subscription.go (4)
16-27: LGTM!The changes to the
Subscriptionstruct fields improve their visibility outside the package and enhance consistency and clarity in naming conventions without altering the core functionality.
32-32: LGTM!The change to the
IDmethod is consistent with the renaming of the struct field and does not alter the functionality of the method.
56-58: LGTM!The renaming of the
Errmethod toErroraligns with common naming practices for error handling in Go. The change is consistent with the renaming of the struct field and does not alter the functionality of the method.
Line range hint
37-52: Verify the usage of theUnsubscribemethod.The changes to the
Unsubscribemethod, particularly the modification of the method signature to accept anEventSubscriberinstead of anEventSystem, are functionally correct. However, it's important to ensure that all calls to this method across the codebase have been updated to pass the correct argument type.Run the following script to verify the usage of the
Unsubscribemethod:Verification successful
Verification Successful:
UnsubscribeMethod Usage is CorrectThe
Unsubscribemethod is being called with the correct argument type,EventSubscriber, across the codebase. The initialization ofapi.eventsusingNewEventSubscriberconfirms this. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `Unsubscribe` method is being called with the correct argument type. # Test: Search for the method usage. Expect: No occurrences of `EventSystem` being passed. rg --type go -A 5 $'Unsubscribe\(.*EventSystem'Length of output: 48
Script:
#!/bin/bash # Description: Verify all calls to the `Unsubscribe` method to ensure they use the correct argument type. # Search for all calls to the `Unsubscribe` method and display the surrounding lines for context. rg --type go -A 5 'Unsubscribe\('Length of output: 4176
Script:
#!/bin/bash # Description: Verify the type of `api.events` to ensure it matches the expected `EventSubscriber`. # Search for the declaration of `api.events` to determine its type. rg --type go 'api\.events'Length of output: 1762
Script:
#!/bin/bash # Description: Locate the declaration or initialization of `api.events` to determine its type. # Search for the declaration or initialization of `api.events`. rg --type go 'events\s*[:=]'Length of output: 841
e2e/evm/test/debug_queries.test.ts (2)
88-97: LGTM! Don't forget to implement the method in EVM.The test case is correctly updated to handle potential errors gracefully. It enhances the robustness of the tests by ensuring that they not only check for successful execution but also handle potential errors.
However, please note that the implementation of the
debug_getBadBlocksmethod in EVM is still pending as indicated by the TODO comment. Make sure to track and complete the implementation.
100-115: LGTM! Don't forget to implement the method in EVM.The test case is correctly updated to handle potential errors gracefully. It enhances the robustness of the tests by ensuring that they not only check for successful execution but also handle potential errors.
However, please note that the implementation of the
debug_storageRangeAtmethod in EVM is still pending as indicated by the TODO comment. Make sure to track and complete the implementation.eth/rpc/rpcapi/event_subscriber_test.go (1)
71-127: LGTM!The
TestParseBloomFromEventsfunction is well-structured and uses a table-driven approach for better readability and maintainability. It covers the important scenarios of parsing bloom from events, including empty events and events with a valid bloom included.The test cases are comprehensive and check for the expected bloom value and error conditions. Great job!
eth/indexer/kv_indexer.go (1)
60-66: LGTM!The change enhances the transaction validation logic by replacing
rpc.TxSuccessOrExpectedFailure(result)withrpc.TxIsValidEnough(result), which provides a more nuanced validation along with a reason for the decision. If the transaction is deemed not valid enough, the code logs a debug message detailing the reason for skipping the indexing of that transaction, including the transaction hash. This improves the clarity of the transaction validation process and provides better logging for debugging purposes, which is consistent with the PR objective.x/evm/evmtest/tx.go (1)
Line range hint
187-220: LGTM!The changes to the
DeployAndExecuteERC20Transferfunction improve code readability and maintainability:
- The function signature now returns named variables, clarifying the purpose of the returned values.
- The assignment of the
predecessorsvariable has been corrected, ensuring the correct value is returned.- The variable
ethTxMsghas been replaced witherc20Transfer, improving code clarity.These changes enhance the overall quality of the code without altering its fundamental logic.
e2e/evm/test/eth_queries.test.ts (1)
2-2: LGTM!The imports from the
etherslibrary are valid and do not introduce any issues.eth/rpc/rpcapi/event_subscriber.go (5)
67-92: LGTM!The
NewEventSubscriberconstructor function looks good. It properly initializes theEventSubscriberfields and starts the necessary goroutines.
197-208: LGTM!The
SubscribeNewHeadsfunction looks good. It correctly creates a new subscription for block headers and calls thesubscribefunction to subscribe to the new block headers events.
211-222: LGTM!The
SubscribePendingTxsfunction looks good. It correctly creates a new subscription for pending transactions and calls thesubscribefunction to subscribe to the new pending transactions events.
227-272: LGTM!The
EventLoopfunction looks good. It correctly handles the installation and uninstallation of filters, manages the associated channels and subscriptions, and processes the mux events.
274-312: LGTM!The
consumeEventsfunction looks good. It correctly consumes events from the Tendermint WebSocket client, unmarshals them, and sends them to the appropriate topic channels based on the event query. The handling of lagging subscribers is a nice addition to prevent blocking the event processing.eth/rpc/backend/call_tx.go (1)
Line range hint
151-166: LGTM!The additional debug logging statements enhance the observability of the
SendRawTransactionfunction by capturing the block height returned in the response after broadcasting the transaction. This change is consistent with the existing logging pattern and provides more detailed insights into the transaction process without altering the core logic of sending a raw transaction.eth/rpc/backend/tx_info.go (2)
25-27: LGTM!The updated comment clarifies the function's behavior when a transaction is not found or has been discarded from a pruning node. The change in the hash comparison logic to use
eth.EthTxHashToString(txHash)may improve consistency and correctness.
319-319: LGTM!The change in the error message formatting improves the clarity of the error message.
eth/rpc/rpcapi/eth_api_test.go (8)
40-44: LGTM!The
NodeSuitestruct changes are part of a larger refactor to rename the test suite. The struct embedssuite.Suiteand contains fields for configuring and running integration tests. Implementing thesuite.TearDownAllSuiteandsuite.SetupAllSuiteinterfaces allows for proper setup and teardown of the test environment.
61-62: LGTM!The changes to
TestSuite_RunAllare part of the test suite refactor to run the newNodeSuitein addition to the existingSuite. This ensures that both test suites are executed.
Line range hint
67-91: LGTM!The
SetupSuitemethod changes are part of the test suite refactor to associate the setup with theNodeSuite. The method initializes the necessary components for running the integration tests, such as the test configuration, network, validator, clients, accounts, and contract data.
94-98: LGTM!The
Test_ChainIDmethod changes are part of the test suite refactor to associate the test with theNodeSuite. The test logic, which verifies the chain ID returned by theeth_chainIdRPC method, remains the same.
Line range hint
101-108: LGTM!The
Test_BlockNumbermethod changes are part of the test suite refactor to associate the test with theNodeSuite. The test logic, which verifies the block number returned by theeth_blockNumberRPC method, remains the same.
Line range hint
111-118: LGTM!The
Test_BlockByNumbermethod changes are part of the test suite refactor to associate the test with theNodeSuite. The test logic, which verifies the block returned by theeth_getBlockByNumberRPC method, remains the same.
Line range hint
121-136: LGTM!The
Test_BalanceAtmethod changes are part of the test suite refactor to associate the test with theNodeSuite. The test logic, which verifies the balance returned by theeth_getBalanceRPC method for a new user and a funded account, remains the same.
Line range hint
177-199: LGTM!The
Test_EstimateGasmethod changes are part of the test suite refactor to associate the test with theNodeSuite. The test logic, which verifies the gas estimation returned by theeth_estimateGasRPC method for different transaction values, remains the same and covers different scenarios.eth/rpc/backend/blocks.go (1)
271-277: Excellent improvements to the transaction validation and logging! LGTM.The changes enhance the transaction validation logic within the
EthMsgsFromTendermintBlockfunction by replacing the previous validation checkrpc.TxSuccessOrExpectedFailurewithrpc.TxIsValidEnough. This allows for more nuanced validation and capturing the specific reason for transaction invalidity.Furthermore, the logger now captures the transaction hash in a more descriptive format using
eth.TmTxHashToString, along with the specific reason for the failure. This significantly improves the clarity of debugging information and facilitates easier troubleshooting of invalid transactions.Overall, these updates strengthen the robustness of the transaction processing logic and provide better insights into transaction outcomes. Great work!
eth/rpc/rpcapi/websockets.go (5)
350-350: LGTM!The change in the
eventsfield type from*rpcfilters.EventSystemto*EventSubscriberis consistent with the refactoring effort to transition to a new event subscription implementation.
359-359: LGTM!The change in the constructor to instantiate
NewEventSubscriberinstead ofrpcfilters.NewEventSystemis consistent with the refactoring effort to transition to a new event subscription implementation.
399-400: LGTM!The change in the event and error channel access methods to use
EventChandError()from the newEventSubscriberimplementation is consistent with the refactoring effort and suggests an enhancement in the event handling mechanism.
572-573: LGTM!The change in the event and error channel access methods to use
EventChandError()from the newEventSubscriberimplementation is consistent with the refactoring effort and suggests an enhancement in the event handling mechanism.
636-637: LGTM!The change in the event and error channel access methods to use
EventChandError()from the newEventSubscriberimplementation is consistent with the refactoring effort and suggests an enhancement in the event handling mechanism.eth/rpc/rpcapi/eth_filters_api.go (5)
1-2: LGTM!The package rename from
filtersapitorpcapialigns with the new directory structure and is a non-breaking change.
14-14: LGTM!The new import
rpcbackendis required for the refactored code that uses therpcbackend.Backendtype.
169-169: LGTM!The change to use the
EventChproperty instead ofeventChis consistent with the update to theSubscriptiontype.
292-292: LGTM!The change to use the
Error()method instead ofErr()is consistent with the update to theSubscriptiontype.
30-39: Refactor looks good, but it's a breaking change.The transition from an interface-based backend (
IFilterEthBackend) to a concrete backend type (*rpcbackend.Backend) simplifies the implementation and enhances type safety. However, it's a breaking change that will require updates to the code that uses theFiltersAPI. Please ensure that all the necessary updates have been made in the codebase to accommodate this change.Run the following script to verify the
FiltersAPIusage:Verification successful
Refactor verified successfully.
The transition from an interface-based backend (
IFilterEthBackend) to a concrete backend type (*rpcbackend.Backend) in theFiltersAPIstruct has been correctly implemented. TheNewBackendfunction returns a*Backend, confirming that the necessary updates have been made in the codebase to accommodate this change. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instantiations of `FiltersAPI` have been updated to pass a `*rpcbackend.Backend`. # Test: Search for `NewImplFiltersAPI` function calls. Expect: Only occurrences with `*rpcbackend.Backend` argument. rg --type go -A 5 $'NewImplFiltersAPI'Length of output: 855
Script:
#!/bin/bash # Description: Verify the type of `evmBackend` to ensure it is `*rpcbackend.Backend`. # Search for the declaration of `evmBackend` to confirm its type. rg --type go -A 2 $'evmBackend'Length of output: 912
Script:
#!/bin/bash # Description: Verify the return type of `backend.NewBackend` to ensure it returns `*rpcbackend.Backend`. # Search for the definition of `backend.NewBackend` to confirm its return type. rg --type go -A 2 $'func NewBackend'Length of output: 177
Script:
#!/bin/bash # Description: Retrieve the complete function signature of `NewBackend` to confirm its return type. # Search for the complete function signature of `NewBackend`. rg --type go -A 5 $'func NewBackend'Length of output: 339
Script:
#!/bin/bash # Description: Retrieve the complete function signature of `NewBackend` to confirm its return type. # Search for the complete function signature of `NewBackend` with more context lines. rg --type go -A 10 $'func NewBackend'Length of output: 577
CHANGELOG.md (1)
120-121: Changes look good, with a few considerations.The addition of the changelog entry for the eth and debug namespace refactor is clear and concise. Removing unused code and improving logging are positive changes that can enhance the codebase's maintainability and debugging capabilities.
To ensure the changes are safe and effective, please verify the following:
Verification successful
Verification mostly successful; consider manual checks for specific deleted code patterns.
The verification script did not find any references to the deleted code patterns, suggesting that the deleted code is not used elsewhere in the codebase. The logging statements appear to follow standard practices. However, if specific patterns for the deleted code are known, a manual check is recommended to ensure thorough verification.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: # 1. Verify that the deleted code is not referenced or used elsewhere in the codebase. # 2. Check if the logging improvements follow best practices and provide meaningful information. # Test 1: Search for references to the deleted code. Expect: No results found. rg -i -C 5 'DeletedCodePattern1|DeletedCodePattern2' # Test 2: Review the logging statements. Expect: Logging follows best practices, providing useful context and information at appropriate levels (info, warn, error). fd --extension go --exec cat {} | rg --type go 'log\.(Info|Warn|Error)'Length of output: 1847
| // TxIsValidEnough returns true if the transaction was successful | ||
| // or if it failed with an ExceedBlockGasLimit error or TxStateDBCommitError error | ||
| func TxSuccessOrExpectedFailure(res *abci.ResponseDeliverTx) bool { | ||
| return res.Code == 0 || TxExceedBlockGasLimit(res) || TxStateDBCommitError(res) | ||
| func TxIsValidEnough(res *abci.ResponseDeliverTx) (condition bool, reason string) { | ||
| if res.Code == 0 { | ||
| return true, "tx succeeded" | ||
| } else if TxExceedBlockGasLimit(res) { | ||
| return true, "tx exceeded block gas limit" | ||
| } else if TxStateDBCommitError(res) { | ||
| return true, "tx state db commit error" | ||
| } | ||
| return false, "unexpected failure" |
There was a problem hiding this comment.
LGTM! The function changes enhance clarity and provide more informative feedback.
The renaming of the function from TxSuccessOrExpectedFailure to TxIsValidEnough and the change in the return type to include a reason string are appropriate modifications that better reflect the purpose and behavior of the function.
The updated logic of checking for specific error conditions (TxExceedBlockGasLimit and TxStateDBCommitError) and returning corresponding reasons enhances the clarity of the function and provides more informative feedback to the caller.
However, the static analysis tool has flagged that the added lines (268-274 and 276) are not covered by tests. To ensure the robustness of the function, it's important to add test cases that cover these lines and validate the expected behavior.
Do you want me to generate the test cases or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 268-274: eth/rpc/rpc.go#L268-L274
Added lines #L268 - L274 were not covered by tests
[warning] 276-276: eth/rpc/rpc.go#L276
Added line #L276 was not covered by tests
| func (es *EventSubscriber) SubscribeLogs(crit filters.FilterCriteria) (*Subscription, pubsub.UnsubscribeFunc, error) { | ||
| var from, to gethrpc.BlockNumber | ||
| if crit.FromBlock == nil { | ||
| from = gethrpc.LatestBlockNumber | ||
| } else { | ||
| from = gethrpc.BlockNumber(crit.FromBlock.Int64()) | ||
| } | ||
| if crit.ToBlock == nil { | ||
| to = gethrpc.LatestBlockNumber | ||
| } else { | ||
| to = gethrpc.BlockNumber(crit.ToBlock.Int64()) | ||
| } | ||
|
|
||
| switch { | ||
| // only interested in new mined logs, mined logs within a specific block range, or | ||
| // logs from a specific block number to new mined blocks | ||
| case (from == gethrpc.LatestBlockNumber && to == gethrpc.LatestBlockNumber), | ||
| (from >= 0 && to >= 0 && to >= from), | ||
| (from >= 0 && to == gethrpc.LatestBlockNumber): | ||
|
|
||
| // Create a subscription that will write all logs matching the | ||
| // given criteria to the given logs channel. | ||
| sub := &Subscription{ | ||
| Id: gethrpc.NewID(), | ||
| Typ: filters.LogsSubscription, | ||
| Event: evmEventsQuery, | ||
| logsCrit: crit, | ||
| Created: time.Now().UTC(), | ||
| Logs: make(chan []*gethcore.Log), | ||
| Installed: make(chan struct{}, 1), | ||
| ErrCh: make(chan error, 1), | ||
| } | ||
| return es.subscribe(sub) | ||
|
|
||
| default: | ||
| return nil, nil, fmt.Errorf("invalid from and to block combination: from > to (%d > %d)", from, to) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add tests to improve coverage.
The SubscribeLogs function implementation looks good. It correctly determines the "from" and "to" block numbers based on the provided filter criteria, validates the block number range, and creates a new subscription with the appropriate fields.
However, the static analysis tool indicates that lines 157-162 are not covered by tests.
To improve the test coverage, consider adding tests that exercise the SubscribeLogs function with different scenarios, such as:
- When the "from" and "to" block numbers are not provided (nil)
- When the "from" and "to" block numbers are provided as specific values
- Error cases when the block number range is invalid
Do you want me to generate the test code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 157-162: eth/rpc/rpcapi/event_subscriber.go#L157-L162
Added lines #L157 - L162 were not covered by tests
| func (s *NodeSuite) Test_SuggestGasPrice() { | ||
| // TODO: the backend method is stubbed to 0 | ||
| _, err := s.ethClient.SuggestGasPrice(context.Background()) | ||
| s.NoError(err) | ||
| } |
There was a problem hiding this comment.
Address the TODO comment.
The Test_SuggestGasPrice method changes are part of the test suite refactor to associate the test with the NodeSuite. The test logic, which verifies the gas price suggestion returned by the eth_gasPrice RPC method, remains the same. However, there is a TODO comment indicating that the backend method is stubbed to 0, which needs to be addressed.
Do you want me to open a GitHub issue to track the task of implementing the proper backend method for gas price suggestion?
| @@ -198,6 +175,7 @@ | |||
| // transaction enters the transaction pool and was signed from one of the | |||
| // transactions this nodes manages. | |||
| func (api *FiltersAPI) NewPendingTransactions(ctx context.Context) (*gethrpc.Subscription, error) { | |||
| api.logger.Debug("eth_newPendingTransactions") | |||
There was a problem hiding this comment.
Add a test to cover the debug log statement.
The debug log statement enhances traceability of operations, which is good. However, the static analysis tool has flagged this line as not being covered by tests. Please add a test case to cover this line and ensure it's being exercised.
Do you want me to generate a test case to cover this line or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 178-178: eth/rpc/rpcapi/eth_filters_api.go#L178
Added line #L178 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (10)
app/evmante/evmante_emit_event.go (1)
57-59: Address the event emission concern and clean up the commented-out line.The comment raises a valid concern about each event being emitted twice. This redundancy can lead to confusion and inefficiencies in event handling. The suggestion to migrate to typed events and align the EVM indexer accordingly is a good recommendation for improving the event system.
Regarding the commented-out line for the
emitted_fromattribute, consider the following:
- If the attribute is not needed, remove the commented-out line to keep the code clean.
- If the attribute provides valuable information, uncomment the line and provide a meaningful value for the attribute to enhance traceability and context for the emitted events.
eth/rpc/rpcapi/event_subscriber.go (1)
274-312: Improve error handling.The
consumeEventsfunction is implemented correctly and follows the expected pattern for consuming events from the Tendermint client. The use of a timeout when sending events to the channel is a good practice to prevent blocking due to lagging subscribers.However, the error handling can be improved. When an event cannot be sent to the channel due to the timeout, consider logging the error or taking appropriate actions to handle the situation gracefully.
Overall, the function is implemented well and serves its purpose of consuming events from the Tendermint client.
eth/rpc/rpcapi/eth_filters_api.go (7)
Line range hint
178-235: LGTM!The function is well-implemented and follows best practices.
The static analysis tool indicates that line 178 is not covered by tests. Do you want me to generate a test case or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 178-178: eth/rpc/rpcapi/eth_filters_api.go#L178
Added line #L178 was not covered by tests
Line range hint
300-355: LGTM!The function is well-implemented and follows best practices.
The static analysis tool indicates that lines 300, 340-343, and 345 are not covered by tests. Do you want me to generate test cases or open a GitHub issue to track this task?
Line range hint
365-425: LGTM!The function is well-implemented and follows best practices.
The static analysis tool indicates that lines 365-366 are not covered by tests. Do you want me to generate test cases or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 365-366: eth/rpc/rpcapi/eth_filters_api.go#L365-L366
Added lines #L365 - L366 were not covered by tests
Line range hint
444-510: LGTM!The function is well-implemented and follows best practices.
The static analysis tool indicates that line 444 is not covered by tests. Do you want me to generate a test case or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 503-503: eth/rpc/rpcapi/eth_filters_api.go#L503
Added line #L503 was not covered by tests
[warning] 520-520: eth/rpc/rpcapi/eth_filters_api.go#L520
Added line #L520 was not covered by tests
[warning] 524-524: eth/rpc/rpcapi/eth_filters_api.go#L524
Added line #L524 was not covered by tests
Line range hint
520-544: LGTM!The function is well-implemented and follows best practices.
The static analysis tool indicates that lines 520, 524, 536, and 539-543 are not covered by tests. Do you want me to generate test cases or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 536-536: eth/rpc/rpcapi/eth_filters_api.go#L536
Added line #L536 was not covered by tests
[warning] 539-543: eth/rpc/rpcapi/eth_filters_api.go#L539-L543
Added lines #L539 - L543 were not covered by tests
Line range hint
578-615: LGTM!The function is well-implemented and follows best practices.
The static analysis tool indicates that lines 578, 594, and 606 are not covered by tests. Do you want me to generate test cases or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 594-594: eth/rpc/rpcapi/eth_filters_api.go#L594
Added line #L594 was not covered by tests
Line range hint
625-655: LGTM!The function is well-implemented and follows best practices.
The static analysis tool indicates that line 625 is not covered by tests. Do you want me to generate a test case or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 625-625: eth/rpc/rpcapi/eth_filters_api.go#L625
Added line #L625 was not covered by testsx/evm/keeper/msg_server.go (1)
53-55: Informative comment highlighting a known issue and suggesting future improvements.The comment provides valuable insights into a known issue where events are emitted twice. It also proposes a future migration to typed events and an alignment of the EVM indexer to resolve this odd behavior. This comment serves as a helpful reminder for the team to address this issue in the future.
Consider creating a GitHub issue to track the future work mentioned in the comment. This will ensure that the task is properly documented, prioritized, and addressed in a timely manner.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (30)
- CHANGELOG.md (1 hunks)
- app/evmante/evmante_emit_event.go (1 hunks)
- e2e/evm/test/basic_queries.test.ts (0 hunks)
- e2e/evm/test/debug_queries.test.ts (1 hunks)
- e2e/evm/test/eth_queries.test.ts (1 hunks)
- eth/indexer/kv_indexer.go (1 hunks)
- eth/rpc/backend/blocks.go (2 hunks)
- eth/rpc/backend/call_tx.go (2 hunks)
- eth/rpc/backend/filters.go (1 hunks)
- eth/rpc/backend/tx_info.go (4 hunks)
- eth/rpc/rpc.go (1 hunks)
- eth/rpc/rpcapi/apis.go (1 hunks)
- eth/rpc/rpcapi/eth_api_test.go (16 hunks)
- eth/rpc/rpcapi/eth_filters_api.go (19 hunks)
- eth/rpc/rpcapi/event_subscriber.go (1 hunks)
- eth/rpc/rpcapi/event_subscriber_test.go (1 hunks)
- eth/rpc/rpcapi/filter_utils.go (2 hunks)
- eth/rpc/rpcapi/filters.go (5 hunks)
- eth/rpc/rpcapi/filtersapi/filter_system.go (0 hunks)
- eth/rpc/rpcapi/filtersapi/filter_system_test.go (0 hunks)
- eth/rpc/rpcapi/net_api_test.go (1 hunks)
- eth/rpc/rpcapi/subscription.go (3 hunks)
- eth/rpc/rpcapi/websockets.go (6 hunks)
- eth/stringify.go (1 hunks)
- eth/stringify_test.go (1 hunks)
- x/common/address.go (1 hunks)
- x/evm/cli/query.go (0 hunks)
- x/evm/evmtest/tx.go (3 hunks)
- x/evm/keeper/hooks.go (1 hunks)
- x/evm/keeper/msg_server.go (1 hunks)
Files not reviewed due to no reviewable changes (4)
- e2e/evm/test/basic_queries.test.ts
- eth/rpc/rpcapi/filtersapi/filter_system.go
- eth/rpc/rpcapi/filtersapi/filter_system_test.go
- x/evm/cli/query.go
Files skipped from review due to trivial changes (2)
- eth/rpc/backend/filters.go
- x/common/address.go
Additional context used
GitHub Check: codecov/patch
eth/rpc/rpc.go
[warning] 268-274: eth/rpc/rpc.go#L268-L274
Added lines #L268 - L274 were not covered by tests
[warning] 276-276: eth/rpc/rpc.go#L276
Added line #L276 was not covered by testseth/rpc/rpcapi/event_subscriber.go
[warning] 96-97: eth/rpc/rpcapi/event_subscriber.go#L96-L97
Added lines #L96 - L97 were not covered by tests
[warning] 102-106: eth/rpc/rpcapi/event_subscriber.go#L102-L106
Added lines #L102 - L106 were not covered by tests
[warning] 108-109: eth/rpc/rpcapi/event_subscriber.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 111-117: eth/rpc/rpcapi/event_subscriber.go#L111-L117
Added lines #L111 - L117 were not covered by tests
[warning] 120-121: eth/rpc/rpcapi/event_subscriber.go#L120-L121
Added lines #L120 - L121 were not covered by tests
[warning] 125-133: eth/rpc/rpcapi/event_subscriber.go#L125-L133
Added lines #L125 - L133 were not covered by tests
[warning] 136-138: eth/rpc/rpcapi/event_subscriber.go#L136-L138
Added lines #L136 - L138 were not covered by tests
[warning] 142-143: eth/rpc/rpcapi/event_subscriber.go#L142-L143
Added lines #L142 - L143 were not covered by tests
[warning] 145-147: eth/rpc/rpcapi/event_subscriber.go#L145-L147
Added lines #L145 - L147 were not covered by tests
[warning] 150-151: eth/rpc/rpcapi/event_subscriber.go#L150-L151
Added lines #L150 - L151 were not covered by tests
[warning] 157-162: eth/rpc/rpcapi/event_subscriber.go#L157-L162
Added lines #L157 - L162 were not covered by testseth/rpc/backend/tx_info.go
[warning] 60-60: eth/rpc/backend/tx_info.go#L60
Added line #L60 was not covered by tests
[warning] 357-357: eth/rpc/backend/tx_info.go#L357
Added line #L357 was not covered by testseth/rpc/rpcapi/eth_filters_api.go
[warning] 178-178: eth/rpc/rpcapi/eth_filters_api.go#L178
Added line #L178 was not covered by tests
[warning] 300-300: eth/rpc/rpcapi/eth_filters_api.go#L300
Added line #L300 was not covered by tests
[warning] 340-343: eth/rpc/rpcapi/eth_filters_api.go#L340-L343
Added lines #L340 - L343 were not covered by tests
[warning] 345-345: eth/rpc/rpcapi/eth_filters_api.go#L345
Added line #L345 was not covered by tests
[warning] 365-366: eth/rpc/rpcapi/eth_filters_api.go#L365-L366
Added lines #L365 - L366 were not covered by tests
[warning] 444-444: eth/rpc/rpcapi/eth_filters_api.go#L444
Added line #L444 was not covered by tests
[warning] 503-503: eth/rpc/rpcapi/eth_filters_api.go#L503
Added line #L503 was not covered by tests
[warning] 520-520: eth/rpc/rpcapi/eth_filters_api.go#L520
Added line #L520 was not covered by tests
[warning] 524-524: eth/rpc/rpcapi/eth_filters_api.go#L524
Added line #L524 was not covered by tests
[warning] 536-536: eth/rpc/rpcapi/eth_filters_api.go#L536
Added line #L536 was not covered by tests
[warning] 539-543: eth/rpc/rpcapi/eth_filters_api.go#L539-L543
Added lines #L539 - L543 were not covered by tests
[warning] 578-578: eth/rpc/rpcapi/eth_filters_api.go#L578
Added line #L578 was not covered by tests
[warning] 594-594: eth/rpc/rpcapi/eth_filters_api.go#L594
Added line #L594 was not covered by tests
[warning] 606-606: eth/rpc/rpcapi/eth_filters_api.go#L606
Added line #L606 was not covered by tests
[warning] 625-625: eth/rpc/rpcapi/eth_filters_api.go#L625
Added line #L625 was not covered by tests
Additional comments not posted (60)
eth/rpc/rpcapi/net_api_test.go (1)
Line range hint
7-13: LGTM!The test logic is correct and the assertions are validating the expected behavior of the
NetNamespaceAPI. The change in the suite type fromTestSuitetoNodeSuitesuggests a refactoring of the test structure to align the tests more closely with node-specific functionality or behavior, which is a good practice.eth/stringify_test.go (1)
10-23: LGTM!The test function is well-structured and covers important test cases for the
BloomToHexandBloomFromHexfunctions. The test cases are clearly defined and cover various scenarios, including alphanumeric strings, empty strings, and big integers. The test function correctly checks for errors and compares the original and converted bloom filters using thes.Equalfmethod to provide a clear error message in case of a failure.Great job on writing a comprehensive test function!
x/evm/keeper/hooks.go (1)
23-23: LGTM!The change from
eth.BytesToHex(bloom.Bytes())toeth.BloomToHex(bloom)is a good optimization. Using the dedicatedeth.BloomToHexfunction is likely to be more efficient and accurate for converting the bloom filter to a hexadecimal string representation. This change improves the code without altering the overall functionality.eth/stringify.go (4)
14-16: LGTM!The function logic is correct, and the implementation follows the reference from
comet-bft/types/tx.go.
19-21: LGTM!The function logic is correct, and the implementation uses the
Hex()method from thegethcommon.Hashtype.
24-26: LGTM!The function logic is correct, and the implementation uses the
BytesToHexfunction to convert the bloom filter bytes to a hex-encoded string.
29-35: LGTM!The function logic is correct:
- It correctly decodes the hex-encoded bloom filter using the
hex.DecodeStringfunction.- It returns an error if the decoding fails.
- It uses the
gethcore.BytesToBloomfunction to convert the decoded bytes to agethcore.Bloomtype.eth/rpc/rpcapi/subscription.go (5)
16-27: LGTM!The renaming of the struct fields to use exported names improves the clarity and usability of the
Subscriptionstruct. The changes follow Go's naming conventions and are consistent across all fields.
32-32: LGTM!The changes to the
IDmethod are consistent with the renaming of theidfield toId. The method functionality remains the same.
56-58: LGTM!The renaming of the
Errmethod toErroraligns with common naming conventions. The changes are consistent with the renaming of theerrfield toErrCh, and the method functionality remains the same.
Line range hint
2-11: LGTM!The addition of the
coretypesandgethcorepackages to the import statements is consistent with their usage in theSubscriptionstruct fields. The changes are necessary for the proper functioning of the struct.
Line range hint
37-58: Verify the usage ofEventSubscribertype.The changes to the
Unsubscribemethod are consistent with the renaming of the struct fields, and the overall logic remains the same. However, the change in the method signature fromEventSystemtoEventSubscriberindicates a potential change in the event handling mechanism.Run the following script to verify the usage of
EventSubscribertype:Verification successful
Verification Successful:
EventSubscriberType is Correctly UsedThe
EventSubscribertype is defined ineth/rpc/rpcapi/event_subscriber.goand is used consistently across the codebase, including in theUnsubscribemethod. The change fromEventSystemtoEventSubscriberaligns with the event handling mechanism. No issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `EventSubscriber` type. # Test 1: Search for the type definition. echo "Searching for the type definition of 'EventSubscriber'..." ast-grep --lang go --pattern $'type EventSubscriber struct { $$$ }' # Test 2: Search for the usage of the type. echo "Searching for the usage of 'EventSubscriber' type..." rg --type go -w 'EventSubscriber'Length of output: 2832
eth/rpc/rpcapi/apis.go (1)
63-63: LGTM!The change to the initialization of the Filters API service looks good. It aligns with the PR objective of refactoring the Ethereum RPC code by removing unused code segments. The removal of the
filtersapipackage import and the direct call toNewImplFiltersAPIsuggest a consolidation of the Filters API implementation. The overall functionality remains intact, and the change streamlines the code.e2e/evm/test/debug_queries.test.ts (2)
88-97: LGTM!The test case for the
debug_getBadBlocksmethod is well-structured and follows best practices. The use of a try-catch block to handle the expected error is a good approach. The test case correctly expects the method to throw an error with a specific message, which is a good way to test for the absence of the method. The TODO comment is a good reminder for the team to implement the method in the future.
100-115: LGTM!The test case for the
debug_storageRangeAtmethod is well-structured and follows best practices. The use of a try-catch block to handle the expected error is a good approach. The test case correctly expects the method to throw an error with a specific message, which is a good way to test for the absence of the method. The TODO comment is a good reminder for the team to implement the method in the future.eth/rpc/rpcapi/event_subscriber_test.go (2)
31-69: LGTM!The test function
TestEventSubscriberis well-written and covers the essential aspects of theEventSubscriberfunctionality. It checks if the topic channels are created and reused correctly when installing multiple subscriptions for the same event. The test function is comprehensive and does not have any major issues or areas for improvement.
71-127: LGTM!The test function
TestParseBloomFromEventsis well-written and covers different scenarios for testing theParseBloomFromEventsfunction. It uses appropriate test dependencies and emits events using the event manager to simulate real-world scenarios. The test function checks for the presence of the bloom event using thetestutil.AssertEventPresentfunction, ensuring the correctness of the emitted events. The test cases are comprehensive and cover both happy path and error scenarios. The test function is thorough and does not have any major issues or areas for improvement.eth/rpc/rpcapi/filter_utils.go (1)
118-140: LGTM!The
ParseBloomFromEventsfunction is well-implemented and follows the expected logic for extracting bloom data from events. It handles errors appropriately and uses good practices likegogoproto.MessageNamefor event type comparison. The function signature and return values adhere to Go's conventions.Great job on this addition!
eth/indexer/kv_indexer.go (1)
60-66: LGTM!The change from
rpc.TxSuccessOrExpectedFailuretorpc.TxIsValidEnoughenhances the logging capabilities by providing clearer insights into why certain transactions are not indexed. This improves the overall transparency of the indexing process.x/evm/evmtest/tx.go (1)
Line range hint
187-220: LGTM!The changes to the
DeployAndExecuteERC20Transferfunction enhance code clarity and maintainability without altering its core functionality. The modifications include:
- Updating the function signature to return named values
erc20Transferandpredecessors, providing meaningful names to the returned transactions and improving readability.- Refactoring the
predecessorsvariable assignment to align with the new return type structure.- Replacing
ethTxMsgwitherc20Transferto ensure consistent usage of the newly named return value throughout the function.Overall, these changes are beneficial and do not introduce any issues.
eth/rpc/rpcapi/filters.go (5)
2-2: LGTM!The package name change from
filtersapitorpcapiis consistent with the file path and does not affect the functionality of the code.
11-11: LGTM!The import statement for
rpcbackendis necessary to use therpcbackend.Backendtype in the code.
33-33: LGTM!The change of the
backendfield type fromIFilterEthBackendtorpcbackend.Backendis consistent with the import statement change and the modifications in the constructors and internal functions. It suggests a shift in the underlying implementation of the backend functionality.
41-41: LGTM!The changes in the constructors
NewBlockFilterandNewRangeFilterand the internal functionnewFilterto acceptrpcbackend.Backendinstead ofIFilterEthBackendare consistent with thebackendfield type change in theFilterstruct. The changes do not affect the functionality of the constructors and the internal function.Also applies to: 48-48, 81-86
195-195: LGTM!The change in the method call from
backend.GetLogsFromBlockResultstorpcbackend.GetLogsFromBlockResultsis consistent with the shift in the underlying implementation of the backend functionality. The change does not affect the functionality of the method call.eth/rpc/rpc.go (1)
266-276: LGTM! The changes to the function signature and logic improve clarity and usability.The renaming of the function to
TxIsValidEnoughbetter reflects its purpose. Returning a tuple of a boolean condition and a string reason provides more informative output about the transaction's outcome.The explicit checks for transaction success, exceeding block gas limit, and state database commit error, along with returning corresponding reason strings, allow for clearer understanding of the transaction's outcome.
Tools
GitHub Check: codecov/patch
[warning] 268-274: eth/rpc/rpc.go#L268-L274
Added lines #L268 - L274 were not covered by tests
[warning] 276-276: eth/rpc/rpc.go#L276
Added line #L276 was not covered by testse2e/evm/test/eth_queries.test.ts (1)
2-2: Verify the usage of imported functions and remove unused imports.The imported functions
parseEther,keccak256, andAbiCoderfrom theetherslibrary were likely used in the removed test case foreth_getProof. Please verify if these functions are used in other parts of the file. If they are not used anywhere else, consider removing the imports to keep the code clean and maintainable.Run the following script to verify the usage of the imported functions:
If the script confirms that the imported functions are not used anywhere else in the file, apply this diff to remove the unused imports:
-import { parseEther, keccak256, AbiCoder } from "ethers" +import { parseEther } from "ethers"Verification successful
Imports are necessary and should not be removed.
The imported functions
parseEther,keccak256, andAbiCoderfrom theetherslibrary are actively used in the filee2e/evm/test/eth_queries.test.ts. Therefore, these imports should be retained. No changes are needed regarding these imports.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the imported functions are used in the file. # Test: Search for the usage of `parseEther`. Expect: No occurrences if only used in the removed test case. rg --type typescript -w parseEther e2e/evm/test/eth_queries.test.ts # Test: Search for the usage of `keccak256`. Expect: No occurrences if only used in the removed test case. rg --type typescript -w keccak256 e2e/evm/test/eth_queries.test.ts # Test: Search for the usage of `AbiCoder`. Expect: No occurrences if only used in the removed test case. rg --type typescript -w AbiCoder e2e/evm/test/eth_queries.test.tsLength of output: 311
Script:
#!/bin/bash # Description: Verify if the imported functions are used in the file. # Test: Search for the usage of `parseEther`. Expect: No occurrences if only used in the removed test case. rg --type ts -w parseEther e2e/evm/test/eth_queries.test.ts # Test: Search for the usage of `keccak256`. Expect: No occurrences if only used in the removed test case. rg --type ts -w keccak256 e2e/evm/test/eth_queries.test.ts # Test: Search for the usage of `AbiCoder`. Expect: No occurrences if only used in the removed test case. rg --type ts -w AbiCoder e2e/evm/test/eth_queries.test.tsLength of output: 681
eth/rpc/rpcapi/event_subscriber.go (5)
67-92: LGTM!The
NewEventSubscriberconstructor function is implemented correctly. It properly initializes the fields of theEventSubscriberstruct and starts the necessary goroutines.
157-194: LGTM!The
SubscribeLogsfunction is implemented correctly and follows the expected pattern for creating a logs subscription. The validation of the "from" and "to" block combination is a good practice to prevent invalid subscriptions.Note: The static analysis tool has flagged that lines 157-162 are not covered by tests. While this is not a critical issue as it only affects the initialization of the "from" and "to" block numbers, consider adding tests for completeness.
Tools
GitHub Check: codecov/patch
[warning] 157-162: eth/rpc/rpcapi/event_subscriber.go#L157-L162
Added lines #L157 - L162 were not covered by tests
197-208: LGTM!The
SubscribeNewHeadsfunction is implemented correctly and follows the expected pattern for subscribing to new block headers events.
211-222: LGTM!The
SubscribePendingTxsfunction is implemented correctly and follows the expected pattern for subscribing to new pending transactions events.
227-272: LGTM!The
EventLoopfunction is implemented correctly and follows the expected pattern for managing subscriptions. The use of a mutex to protect the access to the index and topic channels is a good practice to prevent race conditions. The check for whether a topic is used by other subscriptions before unsubscribing is a good optimization.eth/rpc/backend/call_tx.go (1)
151-153: LGTM!The changes enhance the logging capabilities of the
SendRawTransactionfunction without altering its core functionality. The additional debug logs provide better insights into the transaction process by including the transaction hash and the block height at which the transaction was included.These improvements will aid in debugging and monitoring the transaction lifecycle.
Also applies to: 164-166
eth/rpc/backend/tx_info.go (2)
25-27: LGTM!The updated documentation provides clearer expectations for users regarding the function's behavior in edge cases. The change in transaction hash comparison may also improve consistency.
319-319: LGTM!The refined error message formatting enhances readability.
eth/rpc/rpcapi/eth_api_test.go (14)
61-62: LGTM!The changes to run the
NodeSuitein addition to theSuiteare consistent with the test suite refactoring.
Line range hint
67-91: LGTM!The changes to associate the
SetupSuitefunction with theNodeSuitestruct are consistent with the test suite refactoring.
94-98: LGTM!The changes to associate the
Test_ChainIDfunction with theNodeSuitestruct are consistent with the test suite refactoring.
Line range hint
101-108: LGTM!The changes to associate the
Test_BlockNumberfunction with theNodeSuitestruct are consistent with the test suite refactoring.
Line range hint
111-118: LGTM!The changes to associate the
Test_BlockByNumberfunction with theNodeSuitestruct are consistent with the test suite refactoring.
Line range hint
121-135: LGTM!The changes to associate the
Test_BalanceAtfunction with theNodeSuitestruct are consistent with the test suite refactoring.
Line range hint
138-145: LGTM!The changes to associate the
Test_StorageAtfunction with theNodeSuitestruct are consistent with the test suite refactoring.
Line range hint
148-156: LGTM!The changes to associate the
Test_PendingStorageAtfunction with theNodeSuitestruct are consistent with the test suite refactoring.
Line range hint
159-165: LGTM!The changes to associate the
Test_CodeAtfunction with theNodeSuitestruct are consistent with the test suite refactoring.
Line range hint
168-174: LGTM!The changes to associate the
Test_PendingCodeAtfunction with theNodeSuitestruct are consistent with the test suite refactoring.
Line range hint
177-198: LGTM!The changes to associate the
Test_EstimateGasfunction with theNodeSuitestruct are consistent with the test suite refactoring.
201-205: LGTM!The changes to associate the
Test_SuggestGasPricefunction with theNodeSuitestruct are consistent with the test suite refactoring.
Line range hint
208-306: LGTM!The changes to associate the
Test_SimpleTransferTransactionfunction with theNodeSuitestruct and the enhancements to the test logic are consistent with the test suite refactoring and the improvements mentioned in the summary.
Line range hint
311-428: LGTM!The changes to associate the
Test_SmartContractfunction with theNodeSuitestruct and the minor enhancements to the test logic are consistent with the test suite refactoring mentioned in the summary.eth/rpc/backend/blocks.go (1)
271-277: Approve the enhanced transaction validation and logging.The changes introduce a more nuanced approach to validating transaction results by using
rpc.TxIsValidEnough, which not only checks if a transaction is valid but also provides a reason for its validity status. The logging mechanism is enhanced to provide more context about why a transaction was deemed invalid, including the transaction hash and the reason for invalidity. These changes align with the PR objective of improving logging in the eth namespace.Verify that
rpc.TxIsValidEnoughis used consistently throughout the codebase for validating transaction results. Run the following script:Verification successful
Consistent usage of
rpc.TxIsValidEnoughconfirmed.The function
rpc.TxIsValidEnoughis used consistently across the codebase for validating transaction results, as evidenced by its presence in multiple files. This aligns with the intended approach for transaction validation. No issues were found regarding its usage.
- Locations where
rpc.TxIsValidEnoughis used:
eth/rpc/backend/tx_info.goeth/indexer/kv_indexer.goeth/rpc/backend/blocks.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify `rpc.TxIsValidEnough` is used consistently for validating transaction results. # Test: Search for the function usage. Expect: Only occurrences of `rpc.TxIsValidEnough`. rg --type go -A 5 $'rpc\\.Tx(Success|Failure)'Length of output: 46
Script:
#!/bin/bash # Description: Verify `rpc.TxIsValidEnough` is used consistently for validating transaction results. # Test: Search for the function usage. Expect: Occurrences of `rpc.TxIsValidEnough`. rg --type go 'rpc\.TxIsValidEnough'Length of output: 298
eth/rpc/rpcapi/websockets.go (6)
350-350: LGTM! Ensure consistency with this change throughout the file.The change in the
eventsfield type from*rpcfilters.EventSystemto*EventSubscriberindicates a shift towards a new event subscription mechanism. Make sure the rest of the code changes in this file are consistent with this change.
359-359: LGTM!The change in the instantiation of the
pubSubAPIto useNewEventSubscriberinstead ofrpcfilters.NewEventSystemis consistent with the previous update to thepubSubAPIstruct.
399-400: LGTM!The changes to access the event channel via
sub.EventChand the error channel viasub.Error()are consistent with the shift towards the newEventSubscriberimplementation. These changes should improve the clarity and efficiency of event handling within the WebSocket server.
572-573: LGTM!The changes to access the event channel via
sub.EventChand the error channel viasub.Error()in thesubscribeLogsmethod are consistent with the updates made to thesubscribeNewHeadsmethod. They reflect the shift towards the newEventSubscriberimplementation for managing event subscriptions.
593-593: LGTM! Enhanced log filtering flexibility.The addition of
crit.FromBlockandcrit.ToBlockarguments to theFilterLogsfunction call enhances the flexibility of the log filtering mechanism. Users can now specify a specific block range for filtering logs, providing more control over the returned results.
636-637: LGTM!The changes to access the event channel via
sub.EventChand the error channel viasub.Error()in thesubscribePendingTransactionsmethod are consistent with the updates made to the other subscription methods. They reflect the shift towards the newEventSubscriberimplementation for managing event subscriptions.eth/rpc/rpcapi/eth_filters_api.go (3)
Line range hint
56-74: LGTM!The function is well-implemented and follows best practices.
Line range hint
108-171: LGTM!The function is well-implemented and follows best practices.
Line range hint
245-294: LGTM!The function is well-implemented and follows best practices.
Tools
GitHub Check: codecov/patch
[warning] 300-300: eth/rpc/rpcapi/eth_filters_api.go#L300
Added line #L300 was not covered by testsCHANGELOG.md (1)
120-120: Looks good!The new changelog entry clearly summarizes the refactor, mentioning the affected namespaces (eth and debug) and the nature of the changes (deleting unused code and improving logging). This provides a helpful overview for users to understand the scope and impact of the PR.
| func TxIsValidEnough(res *abci.ResponseDeliverTx) (condition bool, reason string) { | ||
| if res.Code == 0 { | ||
| return true, "tx succeeded" | ||
| } else if TxExceedBlockGasLimit(res) { | ||
| return true, "tx exceeded block gas limit" | ||
| } else if TxStateDBCommitError(res) { | ||
| return true, "tx state db commit error" | ||
| } | ||
| return false, "unexpected failure" |
There was a problem hiding this comment.
Add unit tests to cover the different conditions and ensure correctness.
The static analysis hints rightfully point out the lack of test coverage for the added lines.
It's important to add unit tests to cover the different conditions (res.Code == 0, TxExceedBlockGasLimit(res), TxStateDBCommitError(res), and the default case) and ensure the correctness of the function. Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 268-274: eth/rpc/rpc.go#L268-L274
Added lines #L268 - L274 were not covered by tests
[warning] 276-276: eth/rpc/rpc.go#L276
Added line #L276 was not covered by tests
| func (es *EventSubscriber) WithContext(ctx context.Context) { | ||
| es.Ctx = ctx | ||
| } |
There was a problem hiding this comment.
Add tests to improve coverage.
The WithContext function is implemented correctly. However, the static analysis tool has flagged that lines 96-97 are not covered by tests.
To improve the test coverage, consider adding unit tests for the WithContext function. Do you want me to suggest some test cases or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 96-97: eth/rpc/rpcapi/event_subscriber.go#L96-L97
Added lines #L96 - L97 were not covered by tests
| func (es *EventSubscriber) subscribe(sub *Subscription) (*Subscription, pubsub.UnsubscribeFunc, error) { | ||
| var ( | ||
| err error | ||
| cancelFn context.CancelFunc | ||
| ) | ||
|
|
||
| ctx, cancelFn := context.WithCancel(context.Background()) | ||
| defer cancelFn() | ||
|
|
||
| existingSubs := es.EventBus.Topics() | ||
| for _, topic := range existingSubs { | ||
| if topic == sub.Event { | ||
| eventCh, unsubFn, err := es.EventBus.Subscribe(sub.Event) | ||
| if err != nil { | ||
| err := errors.Wrapf(err, "failed to subscribe to topic: %s", sub.Event) | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| sub.EventCh = eventCh | ||
| return sub, unsubFn, nil | ||
| } | ||
| } | ||
|
|
||
| switch sub.Typ { | ||
| case filters.LogsSubscription: | ||
| err = es.TmWSClient.Subscribe(ctx, sub.Event) | ||
| case filters.BlocksSubscription: | ||
| err = es.TmWSClient.Subscribe(ctx, sub.Event) | ||
| case filters.PendingTransactionsSubscription: | ||
| err = es.TmWSClient.Subscribe(ctx, sub.Event) | ||
| default: | ||
| err = fmt.Errorf("invalid filter subscription type %d", sub.Typ) | ||
| } | ||
|
|
||
| if err != nil { | ||
| sub.ErrCh <- err | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| // wrap events in a go routine to prevent blocking | ||
| es.Install <- sub | ||
| <-sub.Installed | ||
|
|
||
| eventCh, unsubFn, err := es.EventBus.Subscribe(sub.Event) | ||
| if err != nil { | ||
| return nil, nil, errors.Wrapf(err, "failed to subscribe to topic after installed: %s", sub.Event) | ||
| } | ||
|
|
||
| sub.EventCh = eventCh | ||
| return sub, unsubFn, nil | ||
| } |
There was a problem hiding this comment.
Improve test coverage and error handling.
The subscribe function is implemented correctly and handles the subscription logic based on the subscription type and existing subscriptions. However, there are a few areas that can be improved:
- Test Coverage:
The static analysis tool has flagged that several code blocks within this function are not covered by tests, such as lines 102-106, 108-109, 111-117, 120-121, 125-133, 136-138, 142-143, 145-147, and 150-151.
To improve the test coverage, consider adding unit tests for the different code paths and edge cases in the subscribe function. Do you want me to suggest some test cases or open a GitHub issue to track this task?
- Error Handling:
The error handling can be improved by wrapping the errors with more context. For example:
if err != nil {
return nil, nil, errors.Wrap(err, "failed to subscribe to Tendermint event")
}This will provide more meaningful error messages when debugging issues.
Overall, the function is implemented correctly and follows the expected subscription logic.
Tools
GitHub Check: codecov/patch
[warning] 102-106: eth/rpc/rpcapi/event_subscriber.go#L102-L106
Added lines #L102 - L106 were not covered by tests
[warning] 108-109: eth/rpc/rpcapi/event_subscriber.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 111-117: eth/rpc/rpcapi/event_subscriber.go#L111-L117
Added lines #L111 - L117 were not covered by tests
[warning] 120-121: eth/rpc/rpcapi/event_subscriber.go#L120-L121
Added lines #L120 - L121 were not covered by tests
[warning] 125-133: eth/rpc/rpcapi/event_subscriber.go#L125-L133
Added lines #L125 - L133 were not covered by tests
[warning] 136-138: eth/rpc/rpcapi/event_subscriber.go#L136-L138
Added lines #L136 - L138 were not covered by tests
[warning] 142-143: eth/rpc/rpcapi/event_subscriber.go#L142-L143
Added lines #L142 - L143 were not covered by tests
[warning] 145-147: eth/rpc/rpcapi/event_subscriber.go#L145-L147
Added lines #L145 - L147 were not covered by tests
[warning] 150-151: eth/rpc/rpcapi/event_subscriber.go#L150-L151
Added lines #L150 - L151 were not covered by tests
| isValidEnough, reason := rpc.TxIsValidEnough(&txResult.TxResult) | ||
| if !isValidEnough { | ||
| return nil, errors.Errorf("invalid ethereum tx: %s", reason) |
There was a problem hiding this comment.
Improve test coverage for the updated error message.
The change in validation logic to provide more informative error messages is a good improvement as it can aid in debugging and understanding transaction failures.
However, the added error message on line 357 is not covered by tests according to the static analysis report. It's important to have adequate test coverage for error scenarios.
Do you want me to suggest some test cases or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 357-357: eth/rpc/backend/tx_info.go#L357
Added line #L357 was not covered by tests
| @@ -57,7 +57,7 @@ | |||
| // Fallback to find tx index by iterating all valid eth transactions | |||
| msgs := b.EthMsgsFromTendermintBlock(block, blockRes) | |||
| for i := range msgs { | |||
| if msgs[i].Hash == hexTx { | |||
| if msgs[i].Hash == eth.EthTxHashToString(txHash) { | |||
There was a problem hiding this comment.
Improve test coverage for the added line.
The added line 60 is not covered by tests according to the static analysis report. It's important to have adequate test coverage to ensure the robustness of the code.
Do you want me to suggest some test cases or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 60-60: eth/rpc/backend/tx_info.go#L60
Added line #L60 was not covered by tests
Purpose / Abstract
refactor(eth/rpc): Delete unused code and improve logging in the eth and debug namespaces
Related:
Summary by CodeRabbit
New Features
EventSubscriberfor managing blockchain event subscriptions, enhancing real-time monitoring capabilities.Bug Fixes
Refactor
Chores