refactor(rpc-backend): Remove mocks from eth/rpc/backend, partially completing #2037 #2041
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant restructuring across multiple test files and backend functionalities in the Ethereum RPC codebase. Key changes include renaming test packages from Changes
Possibly related issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (8)
eth/rpc/backend/blocks_test.go (3)
13-27: The changes to theTestBlockNumberfunction are valid and improve the test coverage.
- The function correctly tests the
BlockNumbermethod of the backend by retrieving the block number and checking its validity against the latest height.- Consider adding assertions for the latest block and block results retrieved using the RPC client to ensure their correctness.
27-28: Address the TODO comment.The TODO comment indicates that the
GetBlockByHashmethod needs to be tested. Please add tests for this method to ensure its correctness.Do you want me to generate the test code for the
GetBlockByHashmethod or open a GitHub issue to track this task?
29-38: Add assertions for the retrieved block and block results.The code retrieves the latest block and block results using the RPC client, but there are no assertions made on these values. Consider adding assertions to ensure their correctness.
The error handling for the
BlockandBlockResultsmethods ensures that errors are properly checked and reported.eth/rpc/backend/backend.go (1)
63-63: LGTM!The comment change accurately reflects the updated relationship between the
CosmosBackendinterface and the[Backend]interface.Please remember to address the TODO comment and implement the cosmos JSON-RPC defined by Wallet Connect V2 in a future update. Let me know if you need any assistance with that implementation or if you'd like me to open a GitHub issue to track this pending work.
x/common/testutil/testnetwork/start_node.go (1)
163-164: Temporarily disabling the transaction indexer is approved, but ensure the TODO is tracked.Passing
nilinstead ofval.EthTxIndexerto disable the transaction indexing functionality is acceptable as a temporary measure. However, it's important to ensure that:
- The system can still operate correctly without the indexer.
- Any components or features that depend on indexed transaction data are updated accordingly.
Please create a GitHub issue to track the TODO comment and ensure it's addressed in a timely manner to avoid long-term impacts.
Do you want me to open a GitHub issue to track the TODO comment?
eth/rpc/backend/tx_info_test.go (1)
75-82: Remove the commented out assertions or fix them if they are needed.The assertions for checking the receipt fields are commented out. If they are not needed, please remove them to keep the code clean. If they are needed, please fix them and uncomment them to ensure that the receipt fields are properly tested.
x/common/testutil/testnetwork/util.go (1)
Update callers to utilize the new
*sdk.TxResponsereturn valueThe
FillWalletFromValidatorfunction's new signature is consistently used across the codebase, and error checking is properly implemented. However, none of the callers are utilizing the new*sdk.TxResponsereturn value. Consider updating the following files to take advantage of this additional information for improved error handling or logging:
- x/sudo/cli/cli_test.go
- eth/rpc/rpcapi/eth_api_test.go
- eth/rpc/backend/backend_suite_test.go
- x/common/testutil/testnetwork/tx_test.go
Analysis chain
Line range hint
129-143: Verify the function usage in the codebase.The changes to the function signature and error handling improve the clarity and utility of the function by providing more comprehensive return values. However, ensure that all calls to
FillWalletFromValidatorhave been updated to handle the new return values.Run the following script to verify the function usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `FillWalletFromValidator` handle the new return values. # Test: Search for the function usage. Expect: Occurrences handle the `sdk.TxResponse` and error return values. rg --type go -A 5 $'FillWalletFromValidator'Length of output: 3193
x/common/testutil/testnetwork/validator_node.go (1)
270-280: LGTM! Consider additional improvements.The
BlockByEthTxfunction correctly retrieves block results based on an Ethereum transaction hash. It enhances the functionality of theValidatorby integrating Ethereum transaction handling into the existing validator logic.Consider the following improvements:
- Use a non-blank context for production use cases to allow for cancellation and timeouts.
- Add error handling and logging for better observability and debugging.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (26)
- eth/rpc/backend/account_info_test.go (1 hunks)
- eth/rpc/backend/backend.go (1 hunks)
- eth/rpc/backend/backend_suite_test.go (1 hunks)
- eth/rpc/backend/blocks_test.go (1 hunks)
- eth/rpc/backend/call_tx_test.go (1 hunks)
- eth/rpc/backend/chain_info_test.go (1 hunks)
- eth/rpc/backend/client_test.go (1 hunks)
- eth/rpc/backend/evm_query_client_test.go (1 hunks)
- eth/rpc/backend/filters_test.go (1 hunks)
- eth/rpc/backend/mocks/README.md (0 hunks)
- eth/rpc/backend/mocks/client.go (0 hunks)
- eth/rpc/backend/mocks/evm_query_client.go (0 hunks)
- eth/rpc/backend/node_info_test.go (1 hunks)
- eth/rpc/backend/sign_tx_test.go (1 hunks)
- eth/rpc/backend/tracing.go (1 hunks)
- eth/rpc/backend/tracing_test.go (1 hunks)
- eth/rpc/backend/tx_info.go (10 hunks)
- eth/rpc/backend/tx_info_test.go (1 hunks)
- eth/rpc/backend/utils.go (1 hunks)
- eth/rpc/backend/utils_test.go (1 hunks)
- eth/rpc/rpcapi/eth_api_test.go (3 hunks)
- x/common/testutil/testnetwork/start_node.go (1 hunks)
- x/common/testutil/testnetwork/tx_test.go (1 hunks)
- x/common/testutil/testnetwork/util.go (2 hunks)
- x/common/testutil/testnetwork/validator_node.go (2 hunks)
- x/sudo/cli/cli_test.go (1 hunks)
Files not reviewed due to no reviewable changes (3)
- eth/rpc/backend/mocks/README.md
- eth/rpc/backend/mocks/client.go
- eth/rpc/backend/mocks/evm_query_client.go
Files skipped from review due to trivial changes (8)
- eth/rpc/backend/account_info_test.go
- eth/rpc/backend/call_tx_test.go
- eth/rpc/backend/chain_info_test.go
- eth/rpc/backend/evm_query_client_test.go
- eth/rpc/backend/filters_test.go
- eth/rpc/backend/node_info_test.go
- eth/rpc/backend/tracing.go
- eth/rpc/backend/utils.go
Additional comments not posted (32)
eth/rpc/backend/client_test.go (2)
1-1: Approve changing the test package name.Changing the package name from
backendtobackend_testaligns with the Go convention of separating the main package from the test package. This helps keep the main package focused on the core functionality.
1-1: Verify if the removed tests have been replaced.The entire content of the file has been removed, which suggests that the previous test implementations have been either refactored or replaced entirely.
Please ensure that the removed tests are replaced with new, comprehensive tests to avoid a lack of test coverage for the backend functionality.
Run the following script to search for new test files or functions that might have replaced the removed ones:
eth/rpc/backend/utils_test.go (2)
1-1: Package renaming follows best practices.Renaming the package from
backendtobackend_testis a good practice to separate the test code from the main implementation code. This improves the code organization and readability.
1-1: Verify the impact of removing the test cases on the overall testing coverage.The removal of the
mookProofsfunction andTestGetHexProofstest case could potentially reduce the testing coverage for the backend functionalities if they are not replaced or moved to another location.Run the following script to verify the testing coverage:
If the removed test cases are not covered by other tests and the overall testing coverage is reduced, consider adding them back or creating new tests to maintain adequate coverage.
eth/rpc/backend/blocks_test.go (1)
1-1: LGTM!Changing the package name to
backend_testis a good practice to separate test files from the main package code.x/common/testutil/testnetwork/tx_test.go (1)
72-75: LGTM!The error handling has been improved by capturing the error returned from
FillWalletFromValidatorand asserting that it is nil. This enhances the robustness of the test by ensuring that any errors are properly reported and handled.eth/rpc/backend/tracing_test.go (5)
1-21: LGTM!The package name change to
backend_testis a good practice for separating test code from the main package. The introduction of thetraceConfigvariable helps in configuring the tracing functionality consistently across the test functions.
23-56: Great job on restructuring the test cases!The new test case structure with descriptive names and a focus on expected outcomes enhances the clarity and readability of the tests. The simplified logic for handling transaction tracing makes the tests more straightforward and focused on the core functionality being validated. The error handling and assertions are also more concise and effective.
59-100: The updatedTestTraceBlockfunction looks great!The test cases now provide a clear distinction between blocks with and without transactions, improving the test coverage. The handling of transaction counts and block numbers ensures that the tests accurately reflect the state of the blockchain during the tracing process. The assertions verify the expected number of transactions and the tracing results for blocks with transactions, making the tests more comprehensive.
103-127: TheTestTraceCallfunction is implemented correctly.The function retrieves the necessary information (block number, nonce, gas) to construct the transaction arguments correctly. It then calls the
TraceCallfunction with the appropriate arguments, including the tracing configuration. The assertions verify that the tracing results are not nil and match the expected values using theAssertTraceCallfunction, ensuring the correctness of theTraceCallfunctionality.
129-133: TheAssertTraceCallfunction is a useful helper for verifying tracing results.The function correctly asserts the expected values of the trace, including the type, "from" and "to" addresses, and the transaction value. By using the
Requirefunction from the testing package, it ensures that the test fails if any of the assertions fail, providing a clear indication of any discrepancies in the tracing results.eth/rpc/backend/backend_suite_test.go (7)
1-1: Good practice to separate test code from the main implementation.Renaming the package from
backendtobackend_testimproves the clarity and organization of the codebase by separating test code from the main implementation.
31-34: LGTM!The new variables
recipient,amountToSend,transferTxBlockNumber, andtransferTxHashare used to store test data and results. They are initialized with appropriate values and types, and the naming is clear and descriptive.
38-45: LGTM!The new fields added to the
BackendSuitestruct, such ascfg,network,node,fundedAccPrivateKey,fundedAccEthAddr,fundedAccNibiAddr,backend, andethChainID, enhance the suite's capability to handle Ethereum transactions more effectively. The naming is clear and descriptive, and the types are appropriate for the intended usage.
52-80: LGTM!The new method
SetupSuiteadded to theBackendSuitestruct is a good practice to set up the test environment before running the tests. It ensures that the necessary configurations and test data are in place. The method is well-structured and easy to understand, and the error handling is appropriate.
82-109: LGTM!The new method
sendNibiViaEthTransferadded to theBackendSuitestruct facilitates sending Nibi tokens via the Ethereum RPC backend. It encapsulates the logic for transaction creation and sending, leveraging the new Ethereum account setup and ensuring that the transaction is properly signed and sent. The method is well-structured and easy to understand, and the error handling is appropriate. Returning the block number and transaction hash is useful for subsequent tests.
7-13: LGTM!The new import statements for
crypto/ecdsa,github.com/ethereum/go-ethereum/common,github.com/ethereum/go-ethereum/core/types,github.com/ethereum/go-ethereum/crypto, andgithub.com/ethereum/go-ethereum/paramsare necessary for the new test structure and functionality. They provide the required types and functions for handling Ethereum transactions and cryptography.
16-28: LGTM!The new import statements for various packages from the
NibiruChain/nibirurepository, such asgithub.com/NibiruChain/nibiru/v2/x/evm/evmtest,github.com/NibiruChain/nibiru/v2/eth/rpc,github.com/NibiruChain/nibiru/v2/x/evm,github.com/NibiruChain/nibiru/v2/app,github.com/NibiruChain/nibiru/v2/app/appconst,github.com/NibiruChain/nibiru/v2/eth,github.com/NibiruChain/nibiru/v2/eth/rpc/backend,github.com/NibiruChain/nibiru/v2/x/common/testutil/genesis,github.com/NibiruChain/nibiru/v2/x/common/testutil/testapp, andgithub.com/NibiruChain/nibiru/v2/x/common/testutil/testnetwork, are necessary for the new test structure and functionality. They provide the required types and functions for interacting with the Nibiru blockchain and setting up the test environment.eth/rpc/backend/tx_info_test.go (4)
12-45: LGTM!The test cases for
GetTransactionByHashare well-structured and cover the important scenarios. The assertions are comprehensive and verify the expected behavior. The use ofs.Runto run sub-tests is a good practice.
87-130: LGTM!The test cases for
GetTransactionByBlockHashAndIndexare well-structured and cover the important scenarios. The use of theAssertTxResultshelper function is a good practice to avoid code duplication and improve readability.
132-171: LGTM!The test cases for
GetTransactionByBlockNumberAndIndexare well-structured and cover the important scenarios. The use of theAssertTxResultshelper function is a good practice to avoid code duplication and improve readability.
173-179: LGTM!The
AssertTxResultshelper function is a good way to avoid code duplication and improve readability. It checks the important fields of the transaction.x/common/testutil/testnetwork/util.go (1)
145-152: Verify the function usage in the codebase.The changes to the function signature and error handling enhance the function's utility by providing more comprehensive information about the transaction's outcome. However, ensure that all calls to
txOKhave been updated to handle the new return values and error case.Run the following script to verify the function usage:
Verification successful
To ensure a comprehensive verification of the
txOKfunction usage, we need to search for any other occurrences in the entire codebase. Let's run an additional search excluding the file we've already examined.
Changes to
txOKare correctly handled, but consider documenting the updateThe verification process confirms that the changes to the
txOKfunction are correctly handled within the codebase. The function is only used once, and this usage properly handles the new return values (*sdk.TxResponseanderror).However, if
txOKis part of a public API, consider the following:
- Document the change in the function signature in the relevant API documentation or changelog.
- If applicable, update any external documentation or examples that might reference this function.
This will ensure that any potential external users of the function are aware of the change and can update their code accordingly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `txOK` handle the new return values and error case. # Test: Search for the function usage. Expect: Occurrences handle the `sdk.TxResponse` and error return values. rg --type go -A 5 $'txOK'Length of output: 662
Script:
#!/bin/bash # Description: Search for any other occurrences of `txOK` in the codebase, excluding the already examined file. # Test: Search for the function usage in other files. Expect: Either no other occurrences or proper handling of new return values. rg --type go -g '!x/common/testutil/testnetwork/util.go' -A 5 '\btxOK\b'Length of output: 74
x/sudo/cli/cli_test.go (1)
133-137: LGTM!The code change improves the error handling by capturing the error returned by
FillWalletFromValidatorand explicitly checking it usings.NoError(err). This enhances the robustness and reliability of the test suite by ensuring that any error during the wallet funding process is properly handled and reported.eth/rpc/rpcapi/eth_api_test.go (3)
89-92: LGTM!The changes improve the error handling of the wallet funding operation in the test setup. By capturing the error and asserting that no error occurred, the test suite becomes more robust and informative.
225-228: LGTM!The changes enhance the reliability of the test by ensuring that the test account is properly funded before executing the transfer transaction. Capturing the error and asserting that no error occurred improves the test's ability to detect and report issues.
324-327: LGTM!The changes strengthen the test by ensuring that the test account is adequately funded before deploying and interacting with the smart contract. By capturing the error and asserting that no error occurred, the test becomes more resilient and informative in case of funding issues.
eth/rpc/backend/tx_info.go (5)
28-28: LGTM!The function signature change from
common.Hashtogethcommon.Hashimproves clarity by using a more specific type from the Ethereum Go library. This change is consistent with the overall goal of the pull request to refine the existing code to improve type specificity.
93-93: LGTM!The function signature change from
common.Hashtogethcommon.Hashimproves clarity by using a more specific type from the Ethereum Go library. This change is consistent with the overall goal of the pull request to refine the existing code to improve type specificity.
140-140: LGTM!The function signature change from
common.Hashtogethcommon.Hashand the function body change fromcommon.BytesToHashtogethcommon.BytesToHashimprove clarity by using more specific types and functions from the Ethereum Go library. These changes are consistent with the overall goal of the pull request to refine the existing code to improve type specificity.Also applies to: 233-233
266-266: LGTM!The function signature change from
common.Hashtogethcommon.Hashimproves clarity by using a more specific type from the Ethereum Go library. This change is consistent with the overall goal of the pull request to refine the existing code to improve type specificity.
306-306: LGTM!The function signature change from
common.Hashtogethcommon.Hashimproves clarity by using a more specific type from the Ethereum Go library. This change is consistent with the overall goal of the pull request to refine the existing code to improve type specificity.The comments at lines 307-309 provide helpful context about the difference between Tendermint hash and
gethcommon.Hash, which is important for understanding the code.Also applies to: 307-309
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2041 +/- ##
==========================================
- Coverage 66.42% 62.89% -3.54%
==========================================
Files 266 266
Lines 16819 16826 +7
==========================================
- Hits 11172 10582 -590
- Misses 4802 5434 +632
+ Partials 845 810 -35
|
Unique-Divine
changed the title
Unique-Divine
changed the title
Purpose / Abstract
Pulls changes from #2037 to make the PR easier to read.
This change deletes the EVM mock backend.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
backend_testpackage.Chores