Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(evm): backend tests using real transactions #2042

Closed
wants to merge 44 commits into from

Conversation

onikonychev
Copy link
Contributor

@onikonychev onikonychev commented Sep 19, 2024

Summary by CodeRabbit

  • New Features

    • Introduced structured transaction receipts with enhanced fields for clarity.
    • Added new event types for better event handling in the EVM module.
  • Bug Fixes

    • Improved error handling and clarity in transaction log processing.
  • Tests

    • Expanded test coverage for transaction receipts and event handling, ensuring robustness and correctness.
  • Refactor

    • Simplified transaction handling logic by removing unnecessary functions and improving code clarity.
    • Updated event emission to utilize typed events for better structure and safety.

Unique-Divine and others added 30 commits September 9, 2024 14:28
commit a4a2e65
Author: Unique-Divine <[email protected]>
Date:   Fri Sep 13 08:09:44 2024 +0900

    fix: panic on nil gethcore.Log passed to bloom constructor

commit 180c146
Author: Unique-Divine <[email protected]>
Date:   Fri Sep 13 07:34:50 2024 +0900

    refactor: remove unnecessary code

commit bd43152
Author: Unique-Divine <[email protected]>
Date:   Fri Sep 13 07:15:53 2024 +0900

    refactor: remove unused

commit 452d47e
Merge: 8441dde 60167ca
Author: Unique-Divine <[email protected]>
Date:   Fri Sep 13 05:04:59 2024 +0900

    Merge branch 'main' into ud/evm-events

commit 8441dde
Author: Unique-Divine <[email protected]>
Date:   Fri Sep 13 05:04:33 2024 +0900

    chore: changelog

commit 5bcf6a7
Author: Unique-Divine <[email protected]>
Date:   Fri Sep 13 04:57:38 2024 +0900

    remove rpc/backend mocks and add test setup

commit b971a9a
Author: Unique-Divine <[email protected]>
Date:   Fri Sep 13 04:57:07 2024 +0900

    refactor(evm): use typed event for tx logs and message emission

commit 255305b
Author: Unique-Divine <[email protected]>
Date:   Fri Sep 13 02:14:47 2024 +0900

    chore: linter

commit 2f8b1e4
Author: Unique-Divine <[email protected]>
Date:   Fri Sep 13 02:09:10 2024 +0900

    chore: stopping point

commit 1df1e9d
Author: Unique-Divine <[email protected]>
Date:   Fri Sep 13 00:55:33 2024 +0900

    wip!: save

commit a580362
Author: Unique-Divine <[email protected]>
Date:   Thu Sep 12 09:55:48 2024 +0900

    chore: linter and changelog

commit 121287d
Merge: f92de5e 6ba79bb
Author: Unique-Divine <[email protected]>
Date:   Thu Sep 12 09:47:00 2024 +0900

    Merge branch 'main' into ud/2408/eth-rpc

commit f92de5e
Author: Unique-Divine <[email protected]>
Date:   Wed Sep 11 13:38:09 2024 +0900

    chore: changelog

commit 1390371
Merge: 0b4b39b 64beba7
Author: Unique-Divine <[email protected]>
Date:   Wed Sep 11 12:21:50 2024 +0900

    Merge branch 'main' into ud/2408/eth-rpc

commit 0b4b39b
Author: Unique-Divine <[email protected]>
Date:   Wed Sep 11 11:55:23 2024 +0900

    chore(github): Automate labeleing and add-to-project for GH issues/tickets

commit 94519d4
Author: Unique-Divine <[email protected]>
Date:   Tue Sep 10 23:29:48 2024 +0900

    pending chekcpoint

commit 563c253
Author: Unique-Divine <[email protected]>
Date:   Mon Sep 9 15:02:02 2024 +0900

    test(e2e): remove unnecessary skips

commit bb6cb20
Author: Unique-Divine <[email protected]>
Date:   Mon Sep 9 14:36:59 2024 +0900

    remove dead code

commit 97da983
Merge: 0868ce8 b1aac01
Author: Unique-Divine <[email protected]>
Date:   Mon Sep 9 14:28:59 2024 +0900

    Merge branch 'main' into ud/2408/eth-rpc

commit 0868ce8
Author: Unique-Divine <[email protected]>
Date:   Mon Sep 9 14:28:53 2024 +0900

    wip!: checkpoint
@onikonychev onikonychev requested a review from a team as a code owner September 19, 2024 09:12
Copy link
Contributor

coderabbitai bot commented Sep 19, 2024

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes involve significant updates to the Ethereum backend, including the introduction of new test cases, modifications to existing function signatures, and the removal of obsolete functions. Key alterations include the transition to typed events for better event handling, updates to chain ID parsing, and enhancements in transaction receipt structures. The overall structure of the codebase has been streamlined for clarity and maintainability, with a focus on improving type safety and reducing reliance on generic data types.

Changes

File(s) Change Summary
CHANGELOG.md Added a new test case for EVM RPC backend, documenting improvements and code refactors.
app/appconst/appconst.go Updated knownEthChainIDMap to use constants instead of hardcoded integers for Ethereum chain IDs.
app/server/config/server_config.go Removed FixRevertGasRefundHeight configuration option and associated comments.
eth/chain_id.go Modified ParseEthChainID function to return only *big.Int, removing error handling.
eth/eip712/encoding.go, eth/eip712/encoding_legacy.go Simplified error handling related to chain ID parsing, allowing potential invalid chain IDs to be processed.
eth/rpc/backend/account_info.go, eth/rpc/backend/blocks.go, eth/rpc/backend/call_tx.go, eth/rpc/backend/node_info.go, eth/rpc/backend/tracing.go Renamed common package references to gethcommon across multiple functions for consistency.
eth/rpc/backend/account_info_test.go, eth/rpc/backend/blocks_test.go, eth/rpc/backend/call_tx_test.go, eth/rpc/backend/chain_info_test.go, eth/rpc/backend/node_info_test.go, eth/rpc/backend/tx_info_test.go, eth/rpc/backend/utils_test.go Added comprehensive test cases for various backend functionalities, enhancing test coverage and reliability.
eth/rpc/backend/filters.go, eth/rpc/backend/sign_tx.go, eth/rpc/backend/evm_query_client_test.go Removed files related to obsolete functionality, streamlining the codebase.
eth/rpc/rpcapi/eth_api.go, eth/rpc/rpcapi/net_api.go Modified API methods to return structured types instead of generic maps and removed unnecessary methods.
x/evm/events.go, x/evm/events_test.go, x/evm/keeper/msg_server.go Introduced new event types and methods for typed event handling, enhancing event processing capabilities.

Possibly related issues

🐰 In the land of code, where changes bloom,
Functions dance and bugs find their doom.
Typed events now take flight,
With tests that shine so bright!
A rabbit hops with joy, oh what a sight!
Code is cleaner, everything feels right!
🌼


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    -- I pushed a fix in commit <commit_id>, please review it.
    -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    -- @coderabbitai generate unit testing code for this file.
    -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Outside diff range and nitpick comments (2)
x/evm/events_test.go (1)

10-34: LGTM! The test function is well-structured and provides good coverage.

The TestFromABCIEvent function follows a clear table-driven testing approach, making it easy to understand and maintain. The test case covers the happy path scenario and uses appropriate assertions to verify the correctness of the FromABCIEvent method.

To further improve the test coverage, consider adding additional test cases to cover edge cases or error scenarios, such as:

  • Test case for handling an invalid ABCI event
  • Test case for handling an ABCI event with missing or incorrect attributes
  • Test case for handling an ABCI event with a different event type

This will help ensure that the FromABCIEvent method is robust and handles various scenarios gracefully.

CHANGELOG.md (1)

123-123: Suggest moving to "Improvements" section and expanding description.

Unless this interface change is breaking, this entry seems better suited under the "Improvements" section than "State Machine Breaking".

Also consider providing a bit more context in the description around what was refactored and why it was unnecessary, to help readers understand the change better.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0929266 and c106b60.

Files selected for processing (36)
  • CHANGELOG.md (1 hunks)
  • app/appconst/appconst.go (1 hunks)
  • app/server/config/server_config.go (1 hunks)
  • eth/chain_id.go (1 hunks)
  • eth/eip712/encoding.go (2 hunks)
  • eth/eip712/encoding_legacy.go (2 hunks)
  • eth/rpc/backend/account_info.go (8 hunks)
  • eth/rpc/backend/account_info_test.go (1 hunks)
  • eth/rpc/backend/backend.go (1 hunks)
  • eth/rpc/backend/backend_suite_test.go (3 hunks)
  • eth/rpc/backend/blocks.go (9 hunks)
  • eth/rpc/backend/blocks_test.go (1 hunks)
  • eth/rpc/backend/call_tx.go (4 hunks)
  • eth/rpc/backend/call_tx_test.go (1 hunks)
  • eth/rpc/backend/chain_info.go (4 hunks)
  • eth/rpc/backend/chain_info_test.go (1 hunks)
  • eth/rpc/backend/evm_query_client_test.go (0 hunks)
  • eth/rpc/backend/filters.go (0 hunks)
  • eth/rpc/backend/filters_test.go (0 hunks)
  • eth/rpc/backend/node_info.go (2 hunks)
  • eth/rpc/backend/node_info_test.go (1 hunks)
  • eth/rpc/backend/sign_tx.go (0 hunks)
  • eth/rpc/backend/sign_tx_test.go (0 hunks)
  • eth/rpc/backend/tracing.go (5 hunks)
  • eth/rpc/backend/tx_info.go (4 hunks)
  • eth/rpc/backend/tx_info_test.go (3 hunks)
  • eth/rpc/backend/utils.go (6 hunks)
  • eth/rpc/backend/utils_test.go (1 hunks)
  • eth/rpc/events_test.go (2 hunks)
  • eth/rpc/rpcapi/eth_api.go (3 hunks)
  • eth/rpc/rpcapi/eth_api_test.go (13 hunks)
  • eth/rpc/rpcapi/net_api.go (1 hunks)
  • eth/rpc/rpcapi/net_api_test.go (1 hunks)
  • x/evm/events.go (2 hunks)
  • x/evm/events_test.go (1 hunks)
  • x/evm/keeper/msg_server.go (1 hunks)
Files not reviewed due to no reviewable changes (5)
  • eth/rpc/backend/evm_query_client_test.go
  • eth/rpc/backend/filters.go
  • eth/rpc/backend/filters_test.go
  • eth/rpc/backend/sign_tx.go
  • eth/rpc/backend/sign_tx_test.go
Files skipped from review due to trivial changes (4)
  • app/server/config/server_config.go
  • eth/chain_id.go
  • eth/rpc/backend/account_info.go
  • eth/rpc/backend/tracing.go
Additional comments not posted (96)
eth/rpc/rpcapi/net_api_test.go (2)

8-8: LGTM!

The change to access the EthRpc_NET API from the node instance (s.node) instead of the validator instance (s.val) is consistent with the refactoring mentioned in the AI-generated summary. This change may improve the accuracy or relevance of the tests being conducted.


11-11: LGTM!

The change to retrieve the chain ID from the node instance (s.node.ClientCtx.ChainID) instead of the validator instance (s.val.ClientCtx.ChainID) is consistent with the refactoring mentioned in the AI-generated summary. This change ensures that the version check for the API is consistent with the new node context introduced in the previous code segment.

eth/rpc/rpcapi/net_api.go (1)

28-28: LGTM!

The networkVersion field is being set correctly using the parsed chainID.

eth/rpc/backend/node_info_test.go (8)

10-15: LGTM!

The TestAccounts function is a well-structured test that verifies the behavior of the Accounts method of the backend. It checks for the absence of errors, the presence of accounts, and the inclusion of the expected validator account. This test provides confidence in the account management functionality of the backend.


17-21: LGTM!

The TestSyncing function is a straightforward test that verifies the behavior of the Syncing method of the backend. It checks for the absence of errors and asserts that the backend is not in a syncing state. This test provides confidence in the accuracy of the synchronization status reported by the backend.


23-25: LGTM!

The TestRPCGasCap function is a simple test that verifies the behavior of the RPCGasCap method of the backend. It asserts that the returned gas cap value matches the default configuration setting. This test provides confidence that the gas cap configuration is being correctly applied in the backend.


27-29: LGTM!

The TestRPCEVMTimeout function is a simple test that verifies the behavior of the RPCEVMTimeout method of the backend. It asserts that the returned EVM timeout value matches the default configuration setting. This test provides confidence that the EVM timeout configuration is being correctly applied in the backend.


31-33: LGTM!

The TestRPCFilterCap function is a simple test that verifies the behavior of the RPCFilterCap method of the backend. It asserts that the returned filter cap value matches the default configuration setting. This test provides confidence that the filter cap configuration is being correctly applied in the backend.


35-37: LGTM!

The TestRPCLogsCap function is a simple test that verifies the behavior of the RPCLogsCap method of the backend. It asserts that the returned logs cap value matches the default configuration setting. This test provides confidence that the logs cap configuration is being correctly applied in the backend.


39-41: LGTM!

The TestRPCBlockRangeCap function is a simple test that verifies the behavior of the RPCBlockRangeCap method of the backend. It asserts that the returned block range cap value matches the default configuration setting. This test provides confidence that the block range cap configuration is being correctly applied in the backend.


43-45: LGTM!

The TestRPCMinGasPrice function is a simple test that verifies the behavior of the RPCMinGasPrice method of the backend. It asserts that the returned minimum gas price value matches the default gas price specified in the eth package. This test provides confidence that the minimum gas price configuration is being correctly applied in the backend.

eth/rpc/backend/utils_test.go (3)

11-23: LGTM! Remember to address the TODO comment.

The test function TestGetLogsFromBlockResults provides good coverage for the GetLogsFromBlockResults function, ensuring that it can retrieve logs from block results without errors.

However, please remember to add proper checks once the structured event emission issue is resolved, as indicated by the TODO comment.


25-53: Excellent test coverage!

The test function TestGetHexProofs provides comprehensive coverage for the GetHexProofs function, ensuring that it handles different scenarios correctly, including cases with no proof, no proof data, and valid proof.

The table-driven test format is a great choice as it makes the test suite maintainable and extensible.


55-68: Nice helper function!

The mockProofs function provides a convenient way to create mock proof operations for testing purposes, allowing control over the number of operations and whether they include proof data.

It encapsulates the logic for creating ProofOps instances, making the test code more readable and maintainable.

x/evm/events.go (4)

4-8: LGTM!

The imports are relevant and required for the code in this file.


14-18: LGTM!

The constants are named appropriately, follow the convention of using the proto message name as the type URL, and are public, which is appropriate for their usage.


29-30: LGTM!

The constant is named appropriately, follows the convention of using the JSON field name, and is public, which is appropriate for its usage.


33-61: LGTM!

The FromABCIEvent methods for EventTxLog and EventBlockBloom types are implemented correctly:

  • The methods are named appropriately and follow the convention of using FromABCIEvent for parsing ABCI events.
  • The methods use the appropriate type URLs for parsing the events.
  • The error handling is comprehensive and provides clear error messages when parsing fails.
  • The methods are public, which is appropriate for their usage.
app/appconst/appconst.go (1)

75-88: LGTM!

The changes improve code readability and maintainability by replacing hardcoded integer values with named constants for the Ethereum chain IDs. The use of constants reduces the risk of errors and makes the code easier to understand and maintain. The functionality remains the same, as the mappings still serve to identify the respective Ethereum chain IDs.

eth/rpc/backend/call_tx_test.go (3)

13-71: LGTM!

The test function TestSetTxDefaults is well-structured and covers a good range of scenarios for setting transaction defaults. The test cases are named appropriately, and the assertions are thorough, validating the expected behavior. The error messages in the test cases provide clear context about the expected behavior.


73-83: LGTM!

The test function TestDoCall is concise and focused on testing the execution of a call transaction. The assertions validate that the response is not nil and the gas usage is greater than zero, ensuring the expected behavior.


85-90: LGTM!

The test function TestGasPrice is concise and focused on testing the gas price returned by the backend. The assertions ensure that the gas price is not nil and is greater than zero, validating the expected behavior.

eth/rpc/backend/chain_info_test.go (8)

13-15: LGTM!

The test function correctly verifies that the chain ID returned by the backend matches the expected value.


17-21: LGTM!

The test function correctly verifies that the chain configuration returned by the backend matches the expected values for the chain ID and London block.


23-29: LGTM!

The test function correctly verifies that the base fee returned by the backend for a specific block matches the expected value.


31-36: LGTM!

The test function correctly verifies that the current header returned by the backend is not nil and has a block number greater than or equal to the expected value.


38-59: LGTM!

The test function correctly verifies that the backend returns pending transactions and that a sent transaction is found in the pending transactions list.


61-81: LGTM!

The test function correctly verifies that the fee history returned by the backend has the expected structure and values, including the lengths of Reward, BaseFee, and GasUsedRatio, and that the GasUsedRatio values are less than or equal to 1.


83-87: LGTM!

The test function correctly verifies that the gas tip cap suggested by the backend is zero.


89-93: LGTM!

The test function correctly verifies that the global minimum gas price returned by the backend is zero.

eth/rpc/backend/node_info.go (4)

7-7: LGTM!

The import path change for the Ethereum common package is consistent with the usage in the rest of the file.


15-16: LGTM!

The changes to the Accounts function are appropriate:

  • The return type has been updated to match the new import path for the Ethereum common package.
  • Returning an empty slice instead of nil is a good practice to avoid nil pointer dereferences.

29-29: LGTM!

The change to use gethcommon.BytesToAddress is consistent with the updated import path for the Ethereum common package.


36-36: LGTM!

The updated comment provides a clearer description of the Syncing function's behavior.

eth/rpc/backend/blocks_test.go (7)

17-20: LGTM!

The test function correctly verifies that the latest block number retrieved from the backend matches the expected value obtained from the network.


22-30: LGTM!

The test function correctly retrieves a block by number and verifies that the block contains the expected properties such as transactions, size, nonce, and block number.


32-36: LGTM!

The test function correctly retrieves a block by hash and verifies its contents using the AssertBlockContents helper function.


38-81: LGTM!

The test function thoroughly tests the BlockNumberFromTendermint function with different test cases, covering scenarios where the block number or hash is specified, as well as an error scenario where neither is specified. The use of sub-tests and structured test cases improves the readability and maintainability of the test code.


83-91: LGTM!

The test function correctly retrieves an Ethereum block by number and verifies that the block contains the expected properties such as block number, transactions, parent hash, and uncle hash.


93-101: LGTM!

The test functions correctly retrieve the transaction count of a block by hash and number, respectively, and assert that the transaction count is greater than zero.


103-108: LGTM!

The AssertBlockContents helper function is a useful utility that encapsulates the common assertions for verifying the contents of a block map. It improves code reuse and readability by centralizing the block content assertions in a single function.

eth/rpc/backend/tx_info_test.go (2)

80-86: LGTM!

The reactivated assertions improve the test coverage by validating the correctness of the transaction receipt fields. This ensures that the GetTransactionReceipt function returns a receipt with the expected properties.


185-218: Excellent addition of the JSON marshaling test!

The TestReceiptMarshalJson function is a valuable addition to the test suite. It ensures that the TransactionReceipt structure can be correctly serialized to and deserialized from JSON format. This test coverage is crucial for maintaining data integrity and compatibility when working with transaction receipts in JSON format.

The test implementation is clear and comprehensive, covering both the gethcore.Receipt and backend.TransactionReceipt types. It also properly checks for errors during the marshaling and unmarshaling processes.

Great work on enhancing the test coverage!

eth/rpc/backend/chain_info.go (4)

180-181: Rephrasing of comment improves clarity.

The rephrasing of the comment for SuggestGasTipCap improves the clarity of the function's purpose and indicates that the function will come to life after implementing tx prioritization. The function logic remains unchanged, so there is no impact on functionality.


148-148: Renaming of method call improves clarity, but verify functionality remains the same.

The renaming of the method call from processBlock to retrieveEVMTxFeesFromBlock improves the clarity of the function's purpose. However, it's important to verify that the retrieveEVMTxFeesFromBlock function has the same functionality as the previous processBlock function to ensure no regressions are introduced.

Run the following script to verify the functionality remains the same:

#!/bin/bash
# Description: Verify `retrieveEVMTxFeesFromBlock` has the same functionality as the previous `processBlock` function.

# Test: Search for the function implementation. Expect: The function to have the same logic as the previous `processBlock` function.
rg --type go -A 20 $'func \(b \*Backend\) retrieveEVMTxFeesFromBlock'

187-191: Verify callers are updated to handle the new return type and confirm the default value is correct.

The change in return type of GlobalMinGasPrice from sdkmath.LegacyDec to *big.Int may impact the callers of this function. Ensure that the callers are updated to handle the new return type.

Also, confirm that big.NewInt(0) is the correct default value for the minimum gas price.

Run the following script to verify the callers are updated and the default value is correct:

#!/bin/bash
# Description: Verify callers are updated to handle the new return type and confirm the default value is correct.

# Test 1: Search for the callers of `GlobalMinGasPrice`. Expect: The callers to handle the new return type.
rg --type go -A 5 $'GlobalMinGasPrice\(\)'

# Test 2: Search for the implementation of the previous default value function. Expect: The previous default value to be equivalent to `big.NewInt(0)`.
rg --type go -A 5 $'func GetDefaultMinGasPrice'

Reminder: Address the TODO comment for dynamic fees.

The TODO comment indicates that dynamic fees are yet to be implemented. Please ensure that this is addressed in a future update.

Do you want me to open a GitHub issue to track this task?


23-24: Simplification of ChainID function looks good, but verify eth.ParseEthChainID always returns a valid chain ID.

The removal of error handling simplifies the function and eliminates unnecessary code. However, it's important to ensure that eth.ParseEthChainID always returns a valid chain ID and doesn't introduce any potential errors that are now being ignored.

Run the following script to verify eth.ParseEthChainID always returns a valid chain ID:

Verification successful

eth.ParseEthChainID always returns a valid chain ID, ensuring robust handling of chain identifiers.

The function delegates the parsing to appconst.GetEthChainID, which uses a map of known chain IDs and returns a default value (ETH_CHAIN_ID_DEFAULT) for unknown chains. This approach guarantees that a valid chain ID is always returned, even for invalid or unknown inputs. The implementation is well-tested, with test cases in eth/chain_id_test.go and app/appconst/appconst_test.go covering various scenarios, including valid and invalid inputs.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify `eth.ParseEthChainID` always returns a valid chain ID.

# Test: Search for the function implementation. Expect: The function to handle all potential error cases and always return a valid chain ID.
rg --type go -A 10 $'func ParseEthChainID'

Length of output: 994


Script:

#!/bin/bash
# Description: Search for ParseEthChainID function or related chain ID parsing logic

# Search for ParseEthChainID function
echo "Searching for ParseEthChainID function:"
rg --type go -A 10 'func ParseEthChainID'

# Search for ChainID usage
echo "Searching for ChainID usage:"
rg --type go -A 5 'ChainID'

# Search for chain ID parsing logic
echo "Searching for chain ID parsing logic:"
rg --type go -A 10 'ParseChainID|ParseBigInt|strconv.ParseUint.*chainID'

Length of output: 85615

eth/rpc/backend/account_info_test.go (5)

14-50: LGTM!

The test function TestGetCode is well-structured and covers the necessary scenarios. The test cases use appropriate assertions to verify the expected behavior of the GetCode method.


52-92: LGTM!

The test function TestGetProof is well-structured and covers the necessary scenarios. The test cases use appropriate assertions to verify the expected behavior of the GetProof method.


94-136: LGTM!

The test function TestGetStorageAt is well-structured and covers the necessary scenarios. The test cases use appropriate assertions to verify the expected behavior of the GetStorageAt method.


138-187: LGTM!

The test function TestGetBalance is well-structured and covers a comprehensive set of scenarios. The test cases use appropriate assertions to verify the expected behavior of the GetBalance method.


190-202: LGTM!

The utility function generateStorageKey correctly generates a storage key from an address and a slot number using the Keccak256 hash function. The implementation follows the expected logic and there are no apparent issues.

eth/rpc/events_test.go (2)

62-62: LGTM!

The change in event type from evm.EventTypeTxLog to evm.TypeUrlEventTxLog enhances the clarity and specificity of the event type used in the test. It does not appear to have any negative impact on the functionality or correctness of the test.


147-147: LGTM!

The change in event type from evm.EventTypeTxLog to evm.TypeUrlEventTxLog enhances the clarity and specificity of the event type used in the test. It does not appear to have any negative impact on the functionality or correctness of the test.

eth/eip712/encoding_legacy.go (1)

164-164: Restore error handling for eth.ParseEthChainID.

As mentioned in the previous comment, removing the error check for eth.ParseEthChainID can lead to issues if an invalid chain ID is passed. It's important to handle the error and return early to prevent the function from continuing with an invalid chainID.

Revert to the previous error handling:

-	chainID := eth.ParseEthChainID(signDoc.ChainId)
+	chainID, err := eth.ParseEthChainID(signDoc.ChainId)
+	if err != nil {
+		return apitypes.TypedData{}, err
+	}
eth/rpc/backend/utils.go (6)

74-75: LGTM!

The updated comments clearly explain the need to manually add uncommitted transactions to the nonce. The logic for handling pending transactions is accurate.


105-108: LGTM!

The function renaming and updated comment enhance code clarity by accurately describing the purpose of retrieving EVM transaction fees and aligning with the eth_feeHistory method's return format.


212-220: LGTM!

The modifications to AllTxLogsFromEvents enhance readability and type safety:

  • Returning both the logs and an error aligns with idiomatic Go practices.
  • Updating the event type check to evm.TypeUrlEventTxLog reflects the new transaction log identification method.
  • Utilizing a typed event structure improves type safety and clarity.

233-235: LGTM!

The modifications to TxLogsFromEvents enhance readability and type safety:

  • Returning both the logs and an error aligns with idiomatic Go practices.
  • Updating the event type check to evm.TypeUrlEventTxLog reflects the new transaction log identification method.
  • Utilizing a typed event structure improves type safety and clarity.

257-266: LGTM!

The updates to ParseTxLogsFromEvent improve the log parsing process:

  • Accepting a pointer to evm.EventTxLog streamlines the function signature.
  • Modifying the parsing logic to work with the new event structure enhances type safety and clarity.

290-296: LGTM!

The update to GetHexProofs improves the function's robustness by handling the case when proofOps is nil, preventing potential nil pointer dereferences. The core logic for iterating over proof operations and encoding the data remains unchanged.

eth/rpc/rpcapi/eth_api_test.go (6)

50-50: LGTM!

The changes to the NodeSuite struct look good:

  • Renaming val to node improves code clarity.
  • Adding txHistory and txBlockHistory fields enables tracking transaction outcomes and corresponding block results.

Also applies to: 60-62


89-91: LGTM!

The changes in SetupSuite to use the node variable instead of val are consistent with the struct update and look good.


241-246: LGTM!

Appending the transaction response to txHistory is consistent with the goal of tracking transaction outcomes and looks good.


287-289: LGTM!

Retrieving the block of the transaction and appending it to txBlockHistory is consistent with the goal of tracking corresponding block results and looks good.

Also applies to: 305-305


336-341: LGTM!

Appending the transaction response to txHistory is consistent with the goal of tracking transaction outcomes and looks good.


Line range hint 406-410: LGTM!

Using the node variable to query the Ethereum account is consistent with the struct update and looks good.

eth/rpc/backend/blocks.go (9)

90-90: LGTM!

The change in the function signature is consistent with the package renaming from common to gethcommon.


118-118: LGTM!

The change in the function signature is consistent with the package renaming from common to gethcommon.


205-205: LGTM!

The change in the function signature is consistent with the package renaming from common to gethcommon.


243-243: LGTM!

The change in the function signature is consistent with the package renaming from common to gethcommon.


371-371: LGTM!

The change to use gethcommon.HexToHash is consistent with the package renaming from common to gethcommon.


381-381: LGTM!

The change to use gethcommon.BytesToHash is consistent with the package renaming from common to gethcommon.


415-415: LGTM!

The change to use gethcommon.Address is consistent with the package renaming from common to gethcommon.


423-423: LGTM!

The change to use gethcommon.BytesToAddress is consistent with the package renaming from common to gethcommon.


Line range hint 1-1: Verify the impact of removing the HeaderByHash function.

The HeaderByHash function, which was responsible for retrieving the block header identified by a hash, has been removed. This suggests a potential shift in how block headers are managed or accessed within the application.

Please ensure that the removal of this function does not break any existing functionality or impact other parts of the codebase. Consider running the following script to search for any references to HeaderByHash and assess the impact:

Verification successful

Removal of HeaderByHash appears intentional with minimal impact.

The HeaderByHash function has been removed, but this change seems to be part of a broader refactoring of the block retrieval API. The codebase still maintains several functions for retrieving blocks and block-related information, including:

  • HeaderByNumber
  • GetBlockByNumber
  • GetBlockByHash
  • EthBlockByNumber

These functions should provide sufficient alternatives for retrieving block headers and other block-related data. Recent commits show ongoing work on block-related functionality, but none specifically address the removal of HeaderByHash, suggesting this change hasn't caused significant issues.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for references to `HeaderByHash` function.

# Test: Search for the function usage. Expect: No occurrences.
rg --type go -A 5 $'HeaderByHash'

Length of output: 32


Script:

#!/bin/bash
# Verify the removal of HeaderByHash and search for potential replacements

# 1. Verify rg command works
echo "Testing rg command:"
rg --type go "func main"

# 2. Case-insensitive search for HeaderByHash
echo -e "\nSearching for HeaderByHash (case-insensitive):"
rg -i --type go "headerby(hash|number)"

# 3. Search for potential replacements
echo -e "\nSearching for potential replacements:"
rg --type go "func.*(Header|Block).*By(Hash|Number)"

# 4. Check recent changes related to block header retrieval
echo -e "\nChecking recent changes:"
git log -n 5 --oneline --grep="header" --grep="block"

Length of output: 4041

eth/rpc/rpcapi/eth_api.go (1)

183-186: Approve changing the return type to *backend.TransactionReceipt.

The shift from returning a generic map[string]interface{} to a structured *backend.TransactionReceipt type is a good change that makes the code more maintainable and less error-prone:

  • It provides a well-defined structure for the receipt data.
  • It enhances clarity and usability of the returned data.

Note: This is a breaking change for any code that relies on the old return type. Ensure all callers are updated to handle the new type.

x/evm/keeper/msg_server.go (2)

88-103: Excellent work on migrating to typed events!

The transition to typed events using ctx.EventManager().EmitTypedEvents is a significant improvement in the event handling mechanism. It enhances the clarity, safety, and structure of the event system.

The evm.EventMessage event, in particular, provides valuable information about the transaction, such as the module name, sender, and transaction type. This will be beneficial for monitoring, analytics, and debugging purposes.

The TODO comment also shows a clear roadmap for migrating the remaining legacy events to typed events, which will further strengthen the event system.


92-97: Temporary commenting of legacy event emission is appropriate.

The commented-out emission of the evm.EventTxLog event, along with the TODO comment referencing the migration issue, indicates a clear intention to migrate this legacy event to a typed event.

Commenting out the legacy event emission during the migration process prevents unnecessary event duplication and maintains a clean event system.

Please ensure that the migration of this event is completed as part of the referenced issue (#2034) to maintain consistency and avoid any potential issues related to the legacy event system.

CHANGELOG.md (2)

122-122: Looks good!

The changelog entry is clear and the categorization under "State Machine Breaking" makes sense for adding new test cases.


Line range hint 1-124: Changelog looks good overall, just a few suggestions for the Unreleased changes:

The Unreleased section of the changelog was reviewed in detail:

Once those suggestions are addressed, the Unreleased changes should be in good shape and provide a nice summary of notable changes for the upcoming release.

The older version changes were skimmed but not reviewed in detail, since the focus is on the current release. Let me know if you would like me to do a more thorough review of those as well.

eth/rpc/backend/backend_suite_test.go (4)

16-17: Addition of necessary import for contract deployment.

The import statement for "github.com/NibiruChain/nibiru/v2/x/evm/embeds" is required for accessing the smart contract's bytecode and ABI used in DeployTestContract.


82-83: Successful funding of test account.

The test account is successfully funded, and the error handling ensures that any issues are caught early.


104-122: Function SendNibiViaEthTransfer correctly sends Ether transactions.

The function appropriately retrieves the nonce and constructs the transaction for transferring Ether. Passing waitForNextBlock enhances flexibility for test scenarios.


146-158: Verify that private keys are securely managed and not exposed.

Ensure that fundedAccPrivateKey is handled securely and is not inadvertently logged or exposed in the codebase, which could lead to security vulnerabilities.

Run the following script to check for any potential exposures:

Verification successful

Private key handling appears secure, used only in test files.

The fundedAccPrivateKey is used appropriately in test files (eth_api_test.go and backend_suite_test.go). It's not exposed through logging, string conversions, or insecure serialization. The key is only used for signing transactions in tests, which is expected behavior.

  • eth/rpc/rpcapi/eth_api_test.go: Defines and initializes the private key for testing.
  • eth/rpc/backend/backend_suite_test.go: Uses the key with gethcore.SignNewTx() for transaction signing in tests.

Note: Ensure that testAccPrivateKey is a designated test key and not a real private key.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that fundedAccPrivateKey is not logged or printed.

# Test: Search for any logging or printing of the private key.
rg --type go 'fundedAccPrivateKey.*(Print|Printf|Println|Fatal|Error|Warn|Info|Debug)'

Length of output: 88


Script:

#!/bin/bash
# Description: Perform a comprehensive check on the usage of fundedAccPrivateKey

# Search for any usage of fundedAccPrivateKey
echo "Searching for any usage of fundedAccPrivateKey:"
rg --type go "fundedAccPrivateKey"

# Check for string conversions or representations
echo -e "\nChecking for string conversions or representations:"
rg --type go "fundedAccPrivateKey.*(.String()|fmt.Sprint|fmt.Sprintf)"

# Look for the private key being passed to functions
echo -e "\nLooking for the private key being passed to functions:"
rg --type go "fundedAccPrivateKey.*\("

# Search for potential serialization or marshaling
echo -e "\nSearching for potential serialization or marshaling:"
rg --type go "fundedAccPrivateKey.*(Marshal|Encode|JSON)"

Length of output: 1259

eth/rpc/backend/call_tx.go (4)

36-36: Approved: Direct field access simplifies the code

The change from b.UnprotectedAllowed() to b.allowUnprotectedTxs simplifies the condition by accessing the field directly. Ensure that allowUnprotectedTxs is properly initialized and that there are no side effects from bypassing the getter method.


158-160: Approved: Added check for args.From to prevent nil pointer dereference

Adding the args.From != nil condition before retrieving the nonce prevents potential nil pointer dereferences when args.From is nil.


205-205: Approved: Using EthPendingBlockNumber for gas estimation

Changing the block number to rpc.EthPendingBlockNumber for gas estimation aligns the estimation with the pending state, which is appropriate for transactions being prepared for submission.


341-341: Approved: Simplified assignment of minimum gas price

Assigning minGasPrice directly to minGasPriceInt improves code clarity and reduces potential for errors in gas price comparisons.

eth/rpc/backend/tx_info.go (7)

242-244: Initialize receipt.Logs to an empty slice when logs are nil.

Good practice initializing receipt.Logs to an empty slice to prevent nil slice issues during JSON marshaling or when iterating over the logs.


247-249: Set ContractAddress correctly for contract creation transactions.

By checking if txData.GetTo() is nil and setting the ContractAddress, you ensure that the receipt accurately reflects the address of newly created contracts.


257-257: Assign EffectiveGasPrice for dynamic fee transactions.

Calculating and setting EffectiveGasPrice using dynamicTx.EffectiveGasPriceWei(baseFee) ensures that the receipt contains the correct gas price, especially important for EIP-1559 transactions.


260-260: Return the pointer to the constructed TransactionReceipt.

Returning the constructed receipt pointer finalizes the GetTransactionReceipt function correctly.


130-141: Verify potential field conflicts when embedding gethcore.Receipt.

By embedding gethcore.Receipt and adding fields like ContractAddress, there is a risk of field overlap if gethcore.Receipt already defines these fields. This could lead to unexpected behavior or JSON marshaling issues.

Run the following script to check for overlapping fields between TransactionReceipt and gethcore.Receipt:

#!/bin/bash
# Description: Check for overlapping fields between TransactionReceipt and gethcore.Receipt

# List fields from gethcore.Receipt
echo "Fields in gethcore.Receipt:"
ast-grep --lang go --pattern 'type Receipt struct { $$$ }' | grep -E '^\s*\w+' | sort

# List fields from TransactionReceipt
echo "Fields in TransactionReceipt:"
ast-grep --lang go --pattern 'type TransactionReceipt struct { $$$ }' eth/rpc/backend/tx_info.go | grep -E '^\s*\w+' | sort

# Compare the fields
echo "Overlapping fields:"
comm -12 <(
  ast-grep --lang go --pattern 'type Receipt struct { $$$ }' | grep -E '^\s*\w+' | sort
) <(
  ast-grep --lang go --pattern 'type TransactionReceipt struct { $$$ }' eth/rpc/backend/tx_info.go | grep -E '^\s*\w+' | sort
)

145-145: Ensure all callers handle the updated return type *TransactionReceipt.

The return type of GetTransactionReceipt has changed from map[string]interface{} to *TransactionReceipt. Verify that all functions calling this method have been updated to handle the new return type to prevent runtime errors.

Run the following script to find all usages of GetTransactionReceipt:

#!/bin/bash
# Description: Find all usages of GetTransactionReceipt

# Search for function calls to GetTransactionReceipt
rg --type go 'GetTransactionReceipt\(' -A 3 -B 3

Review each call site to ensure it properly handles the *TransactionReceipt type.


217-235: Double-check field initialization and potential conflicts in TransactionReceipt.

Ensure that all fields in TransactionReceipt are correctly initialized and that there are no conflicts with embedded fields from gethcore.Receipt. This will prevent any unexpected behavior during JSON marshaling or data access.

Use the following script to identify overlapping fields:

Verification successful

TransactionReceipt struct implementation is correct and conflict-free.

The TransactionReceipt struct in eth/rpc/backend/tx_info.go is properly implemented. It embeds gethcore.Receipt and adds additional fields (ContractAddress, From, To, and EffectiveGasPrice) without any apparent conflicts. The embedding of gethcore.Receipt is a common Go pattern for extending structs, and the usage in the code seems appropriate. Based on the available information, there's no evidence of field initialization issues or conflicts with the embedded gethcore.Receipt.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for overlapping fields in TransactionReceipt initialization

# List fields from gethcore.Receipt
echo "Fields in gethcore.Receipt:"
ast-grep --lang go --pattern 'type Receipt struct { $$$ }' | grep -E '^\s*\w+' | sort

# List fields from TransactionReceipt
echo "Fields in TransactionReceipt:"
ast-grep --lang go --pattern 'type TransactionReceipt struct { $$$ }' eth/rpc/backend/tx_info.go | grep -E '^\s*\w+' | sort

# Identify overlapping fields
echo "Overlapping fields:"
comm -12 <(
  ast-grep --lang go --pattern 'type Receipt struct { $$$ }' | grep -E '^\s*\w+' | sort
) <(
  ast-grep --lang go --pattern 'type TransactionReceipt struct { $$$ }' eth/rpc/backend/tx_info.go | grep -E '^\s*\w+' | sort
)

Length of output: 4461


Script:

#!/bin/bash
# Examine TransactionReceipt and gethcore.Receipt structs

# Display TransactionReceipt struct
echo "TransactionReceipt struct:"
rg -A 10 'type TransactionReceipt struct' eth/rpc/backend/tx_info.go

# Search for gethcore.Receipt definition
echo -e "\ngethcore.Receipt definition:"
rg -A 10 'type Receipt struct' --type go

# Search for usage of gethcore.Receipt in TransactionReceipt
echo -e "\nUsage of gethcore.Receipt in TransactionReceipt:"
rg 'gethcore\.Receipt' eth/rpc/backend/tx_info.go

Length of output: 1110

if err != nil {
panic(err)
}
chainID := eth.ParseEthChainID(clientCtx.ChainID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle the error returned by eth.ParseEthChainID.

The error returned by eth.ParseEthChainID is being ignored, which could lead to a panic if the parsing fails. Handle the error gracefully to prevent unexpected crashes.

Apply this diff to handle the error:

-chainID := eth.ParseEthChainID(clientCtx.ChainID)
+chainID, err := eth.ParseEthChainID(clientCtx.ChainID)
+if err != nil {
+	// TODO: log the error and return an error from the function
+	// For example:
+	// log.Error("failed to parse chain ID", "error", err)
+	// return nil, err
+}

Committable suggestion was skipped due to low confidence.

panic(err)
}

chainID := eth.ParseEthChainID(clientCtx.ChainID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restore error handling for chainID parsing to prevent proceeding with an invalid value.

Removing the error handling for parsing the chainID may lead to proceeding with an invalid chainID if the parsing fails. This could affect subsequent operations that rely on the chainID value.

Consider restoring the error handling to maintain robustness and prevent potential issues:

chainID, err := eth.ParseEthChainID(clientCtx.ChainID)
if err != nil {
    panic(err)
}

if err != nil {
return apitypes.TypedData{}, fmt.Errorf("invalid chain ID passed as argument: %w", err)
}
chainID := eth.ParseEthChainID(signDoc.ChainId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle the potential error returned by eth.ParseEthChainID.

Directly assigning the result of eth.ParseEthChainID to chainID without checking for errors can lead to unexpected behavior if parsing fails. It's important to handle the potential error and return it to the caller for better error handling and debugging.

Consider applying this diff to handle the potential error:

-	chainID := eth.ParseEthChainID(signDoc.ChainId)
+	chainID, err := eth.ParseEthChainID(signDoc.ChainId)
+	if err != nil {
+		return apitypes.TypedData{}, fmt.Errorf("invalid chain ID passed as argument: %w", err)
+	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
chainID := eth.ParseEthChainID(signDoc.ChainId)
chainID, err := eth.ParseEthChainID(signDoc.ChainId)
if err != nil {
return apitypes.TypedData{}, fmt.Errorf("invalid chain ID passed as argument: %w", err)
}

if err != nil {
return apitypes.TypedData{}, errors.New("invalid chain ID passed as argument")
}
chainID := eth.ParseEthChainID(aminoDoc.ChainID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle the potential error returned by eth.ParseEthChainID.

Directly assigning the result of eth.ParseEthChainID to chainID without checking for errors can lead to unexpected behavior if parsing fails. It's important to handle the potential error and return it to the caller for better error handling and debugging.

Consider applying this diff to handle the potential error:

-	chainID := eth.ParseEthChainID(aminoDoc.ChainID)
+	chainID, err := eth.ParseEthChainID(aminoDoc.ChainID)
+	if err != nil {
+		return apitypes.TypedData{}, fmt.Errorf("invalid chain ID passed as argument: %w", err)
+	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
chainID := eth.ParseEthChainID(aminoDoc.ChainID)
chainID, err := eth.ParseEthChainID(aminoDoc.ChainID)
if err != nil {
return apitypes.TypedData{}, fmt.Errorf("invalid chain ID passed as argument: %w", err)
}

if err != nil {
return apitypes.TypedData{}, errors.New("invalid chain ID passed as argument")
}
chainID := eth.ParseEthChainID(aminoDoc.ChainID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restore error handling for eth.ParseEthChainID.

Removing the error check for eth.ParseEthChainID can lead to issues if an invalid chain ID is passed. It's important to handle the error and return early to prevent the function from continuing with an invalid chainID.

Revert to the previous error handling:

-	chainID := eth.ParseEthChainID(aminoDoc.ChainID)
+	chainID, err := eth.ParseEthChainID(aminoDoc.ChainID)
+	if err != nil {
+		return apitypes.TypedData{}, err
+	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
chainID := eth.ParseEthChainID(aminoDoc.ChainID)
chainID, err := eth.ParseEthChainID(aminoDoc.ChainID)
if err != nil {
return apitypes.TypedData{}, err
}

Comment on lines 86 to 100
// Send Transfer TX and use the results in the tests
block, err := s.backend.BlockNumber()
s.Require().NoError(err)
transferTxHash = s.SendNibiViaEthTransfer(recipient, amountToSend, true)
transferTxBlockNumber = rpc.NewBlockNumber(big.NewInt(int64(block) + 1))
blockResults, err := s.backend.TendermintBlockByNumber(transferTxBlockNumber)
s.Require().NoError(err)
s.Require().NotNil(blockResults)
transferTxBlockHash = gethcommon.BytesToHash(blockResults.Block.Hash().Bytes())

// Deploy test erc20 contract
block, err = s.backend.BlockNumber()
s.Require().NoError(err)
_, testContractAddress = s.DeployTestContract(true)
deployContractBlockNumber = rpc.NewBlockNumber(big.NewInt(int64(block) + 1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block number calculation may cause off-by-one errors.

Fetching the block number before sending the transaction and then adding one might not accurately represent the block in which the transaction is included. This could lead to inconsistencies when asserting against transferTxBlockNumber and deployContractBlockNumber.

Consider fetching the block number after the transaction has been processed:

     // Send Transfer TX and use the results in the tests
-    block, err := s.backend.BlockNumber()
-    s.Require().NoError(err)
     transferTxHash = s.SendNibiViaEthTransfer(recipient, amountToSend, true)
+    block, err := s.backend.BlockNumber()
+    s.Require().NoError(err)
     transferTxBlockNumber = rpc.NewBlockNumber(big.NewInt(int64(block)))

Apply a similar change for deploying the test contract at lines 97-100.

Committable suggestion was skipped due to low confidence.

Comment on lines 124 to 145
// DeployTestContract deploys test erc20 contract
func (s *BackendSuite) DeployTestContract(waitForNextBlock bool) (gethcommon.Hash, gethcommon.Address) {
packedArgs, err := embeds.SmartContract_TestERC20.ABI.Pack("")
s.Require().NoError(err)
bytecodeForCall := append(embeds.SmartContract_TestERC20.Bytecode, packedArgs...)
nonce, err := s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
s.Require().NoError(err)

txHash := SendTransaction(
s,
&gethcore.LegacyTx{
Nonce: uint64(*nonce),
Data: bytecodeForCall,
Gas: 1500_000,
GasPrice: big.NewInt(1),
},
waitForNextBlock,
)
contractAddr := crypto.CreateAddress(s.fundedAccEthAddr, (uint64)(*nonce))
return txHash, contractAddr
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use gas estimation for contract deployment to optimize gas usage.

Hardcoding the gas limit to 1,500,000 may not be efficient or sufficient for all contract deployments. Using gas estimation ensures that you're providing the appropriate amount of gas, preventing out-of-gas errors and saving costs.

Modify the function to estimate gas before sending the transaction:

 func (s *BackendSuite) DeployTestContract(waitForNextBlock bool) (gethcommon.Hash, gethcommon.Address) {
     packedArgs, err := embeds.SmartContract_TestERC20.ABI.Pack("")
     s.Require().NoError(err)
     bytecodeForCall := append(embeds.SmartContract_TestERC20.Bytecode, packedArgs...)
     nonce, err := s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
     s.Require().NoError(err)

+    // Estimate gas limit
+    callMsg := ethereum.CallMsg{
+        From: s.fundedAccEthAddr,
+        Data: bytecodeForCall,
+    }
+    gasLimit, err := s.backend.EstimateGas(context.Background(), callMsg)
+    s.Require().NoError(err)
+
     txHash := SendTransaction(
         s,
         &gethcore.LegacyTx{
             Nonce:    uint64(*nonce),
             Data:     bytecodeForCall,
-            Gas:      1500_000,
+            Gas:      gasLimit,
             GasPrice: big.NewInt(1),
         },
         waitForNextBlock,
     )
     contractAddr := crypto.CreateAddress(s.fundedAccEthAddr, (uint64)(*nonce))
     return txHash, contractAddr
 }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// DeployTestContract deploys test erc20 contract
func (s *BackendSuite) DeployTestContract(waitForNextBlock bool) (gethcommon.Hash, gethcommon.Address) {
packedArgs, err := embeds.SmartContract_TestERC20.ABI.Pack("")
s.Require().NoError(err)
bytecodeForCall := append(embeds.SmartContract_TestERC20.Bytecode, packedArgs...)
nonce, err := s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
s.Require().NoError(err)
txHash := SendTransaction(
s,
&gethcore.LegacyTx{
Nonce: uint64(*nonce),
Data: bytecodeForCall,
Gas: 1500_000,
GasPrice: big.NewInt(1),
},
waitForNextBlock,
)
contractAddr := crypto.CreateAddress(s.fundedAccEthAddr, (uint64)(*nonce))
return txHash, contractAddr
}
// DeployTestContract deploys test erc20 contract
func (s *BackendSuite) DeployTestContract(waitForNextBlock bool) (gethcommon.Hash, gethcommon.Address) {
packedArgs, err := embeds.SmartContract_TestERC20.ABI.Pack("")
s.Require().NoError(err)
bytecodeForCall := append(embeds.SmartContract_TestERC20.Bytecode, packedArgs...)
nonce, err := s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
s.Require().NoError(err)
// Estimate gas limit
callMsg := ethereum.CallMsg{
From: s.fundedAccEthAddr,
Data: bytecodeForCall,
}
gasLimit, err := s.backend.EstimateGas(context.Background(), callMsg)
s.Require().NoError(err)
txHash := SendTransaction(
s,
&gethcore.LegacyTx{
Nonce: uint64(*nonce),
Data: bytecodeForCall,
Gas: gasLimit,
GasPrice: big.NewInt(1),
},
waitForNextBlock,
)
contractAddr := crypto.CreateAddress(s.fundedAccEthAddr, (uint64)(*nonce))
return txHash, contractAddr
}

Comment on lines 146 to 158
// SendTransaction signs and sends raw ethereum transaction
func SendTransaction(s *BackendSuite, tx *gethcore.LegacyTx, waitForNextBlock bool) gethcommon.Hash {
signer := gethcore.LatestSignerForChainID(s.ethChainID)
signedTx, err := gethcore.SignNewTx(s.fundedAccPrivateKey, signer, tx)
s.Require().NoError(err)
txBz, err := tx.MarshalBinary()
txBz, err := signedTx.MarshalBinary()
s.Require().NoError(err)
txHash, err := s.backend.SendRawTransaction(txBz)
s.Require().NoError(err)
s.Require().NoError(s.network.WaitForNextBlock())

return rpc.NewBlockNumber(big.NewInt(int64(block) + 1)), txHash
if waitForNextBlock {
s.Require().NoError(s.network.WaitForNextBlock())
}
return txHash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor SendTransaction to return errors instead of failing tests directly.

Using s.Require().NoError(err) within helper functions can cause the entire test suite to fail without clear context. It's better to return errors to the calling function, allowing for more precise error handling and reporting.

Modify the function to return an error:

-func SendTransaction(s *BackendSuite, tx *gethcore.LegacyTx, waitForNextBlock bool) gethcommon.Hash {
+func SendTransaction(s *BackendSuite, tx *gethcore.LegacyTx, waitForNextBlock bool) (gethcommon.Hash, error) {
     signer := gethcore.LatestSignerForChainID(s.ethChainID)
     signedTx, err := gethcore.SignNewTx(s.fundedAccPrivateKey, signer, tx)
-    s.Require().NoError(err)
+    if err != nil {
+        return gethcommon.Hash{}, err
+    }
     txBz, err := signedTx.MarshalBinary()
-    s.Require().NoError(err)
+    if err != nil {
+        return gethcommon.Hash{}, err
+    }
     txHash, err := s.backend.SendRawTransaction(txBz)
-    s.Require().NoError(err)
+    if err != nil {
+        return gethcommon.Hash{}, err
+    }
     if waitForNextBlock {
-        s.Require().NoError(s.network.WaitForNextBlock())
+        err = s.network.WaitForNextBlock()
+        if err != nil {
+            return gethcommon.Hash{}, err
+        }
     }
-    return txHash
+    return txHash, nil
 }

Update calling functions to handle the error:

-   transferTxHash = s.SendNibiViaEthTransfer(recipient, amountToSend, true)
+   transferTxHash, err = s.SendNibiViaEthTransfer(recipient, amountToSend, true)
+   s.Require().NoError(err)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// SendTransaction signs and sends raw ethereum transaction
func SendTransaction(s *BackendSuite, tx *gethcore.LegacyTx, waitForNextBlock bool) gethcommon.Hash {
signer := gethcore.LatestSignerForChainID(s.ethChainID)
signedTx, err := gethcore.SignNewTx(s.fundedAccPrivateKey, signer, tx)
s.Require().NoError(err)
txBz, err := tx.MarshalBinary()
txBz, err := signedTx.MarshalBinary()
s.Require().NoError(err)
txHash, err := s.backend.SendRawTransaction(txBz)
s.Require().NoError(err)
s.Require().NoError(s.network.WaitForNextBlock())
return rpc.NewBlockNumber(big.NewInt(int64(block) + 1)), txHash
if waitForNextBlock {
s.Require().NoError(s.network.WaitForNextBlock())
}
return txHash
// SendTransaction signs and sends raw ethereum transaction
func SendTransaction(s *BackendSuite, tx *gethcore.LegacyTx, waitForNextBlock bool) (gethcommon.Hash, error) {
signer := gethcore.LatestSignerForChainID(s.ethChainID)
signedTx, err := gethcore.SignNewTx(s.fundedAccPrivateKey, signer, tx)
if err != nil {
return gethcommon.Hash{}, err
}
txBz, err := signedTx.MarshalBinary()
if err != nil {
return gethcommon.Hash{}, err
}
txHash, err := s.backend.SendRawTransaction(txBz)
if err != nil {
return gethcommon.Hash{}, err
}
if waitForNextBlock {
err = s.network.WaitForNextBlock()
if err != nil {
return gethcommon.Hash{}, err
}
}
return txHash, nil
}

Comment on lines +115 to +119
Nonce: uint64(*nonce),
Value: amount,
Gas: params.TxGas,
GasPrice: gasPrice,
})
GasPrice: big.NewInt(1),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider dynamic gas price and gas limit for transactions.

Hardcoding Gas: params.TxGas and GasPrice: big.NewInt(1) may not reflect the current network conditions. Using dynamic values ensures that transactions are processed efficiently without overpaying or getting delayed.

Estimate the gas limit and fetch the current gas price:

 func (s *BackendSuite) SendNibiViaEthTransfer(
     to gethcommon.Address,
     amount *big.Int,
     waitForNextBlock bool,
 ) gethcommon.Hash {
     nonce, err := s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
     s.Require().NoError(err)
+    // Estimate gas limit
+    gasLimit, err := s.backend.EstimateGas(context.Background(), ethereum.CallMsg{
+        From: s.fundedAccEthAddr,
+        To:   &to,
+        Value: amount,
+    })
+    s.Require().NoError(err)
+    // Get current gas price
+    gasPrice, err := s.backend.SuggestGasPrice(context.Background())
+    s.Require().NoError(err)
     return SendTransaction(
         s,
         &gethcore.LegacyTx{
             To:       &to,
             Nonce:    uint64(*nonce),
             Value:    amount,
-            Gas:      params.TxGas,
-            GasPrice: big.NewInt(1),
+            Gas:      gasLimit,
+            GasPrice: gasPrice,
         },
         waitForNextBlock,
     )
 }

Committable suggestion was skipped due to low confidence.

Comment on lines +183 to +185
var status uint64 = gethcore.ReceiptStatusSuccessful
if res.Failed {
status = hexutil.Uint(gethcore.ReceiptStatusFailed)
} else {
status = hexutil.Uint(gethcore.ReceiptStatusSuccessful)
}
chainID, err := b.ChainID()
if err != nil {
return nil, err
status = gethcore.ReceiptStatusFailed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify the assignment of status.

You can simplify the initialization of the status variable for better readability.

Apply this diff to streamline the status assignment:

-var status uint64 = gethcore.ReceiptStatusSuccessful
+status := gethcore.ReceiptStatusSuccessful
 if res.Failed {
     status = gethcore.ReceiptStatusFailed
 }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var status uint64 = gethcore.ReceiptStatusSuccessful
if res.Failed {
status = hexutil.Uint(gethcore.ReceiptStatusFailed)
} else {
status = hexutil.Uint(gethcore.ReceiptStatusSuccessful)
}
chainID, err := b.ChainID()
if err != nil {
return nil, err
status = gethcore.ReceiptStatusFailed
status := gethcore.ReceiptStatusSuccessful
if res.Failed {
status = gethcore.ReceiptStatusFailed

@onikonychev onikonychev marked this pull request as draft September 19, 2024 09:25
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 73.10924% with 32 lines in your changes missing coverage. Please review.

Project coverage is 65.67%. Comparing base (0929266) to head (85d1021).

Files with missing lines Patch % Lines
x/evm/events.go 22.72% 15 Missing and 2 partials ⚠️
eth/rpc/backend/utils.go 63.63% 5 Missing and 3 partials ⚠️
eth/rpc/backend/tx_info.go 88.00% 3 Missing ⚠️
eth/chain_id.go 0.00% 2 Missing ⚠️
eth/rpc/backend/blocks.go 87.50% 1 Missing ⚠️
eth/rpc/backend/call_tx.go 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2042      +/-   ##
==========================================
+ Coverage   62.89%   65.67%   +2.77%     
==========================================
  Files         266      265       -1     
  Lines       16826    16662     -164     
==========================================
+ Hits        10582    10942     +360     
+ Misses       5434     4828     -606     
- Partials      810      892      +82     
Files with missing lines Coverage Δ
app/appconst/appconst.go 57.14% <ø> (ø)
eth/eip712/encoding.go 80.00% <100.00%> (+2.80%) ⬆️
eth/eip712/encoding_legacy.go 78.78% <100.00%> (+2.31%) ⬆️
eth/rpc/backend/account_info.go 56.25% <100.00%> (+45.53%) ⬆️
eth/rpc/backend/backend.go 85.71% <100.00%> (+10.71%) ⬆️
eth/rpc/backend/chain_info.go 64.36% <100.00%> (+47.87%) ⬆️
eth/rpc/backend/node_info.go 68.29% <100.00%> (+64.03%) ⬆️
eth/rpc/backend/tracing.go 53.14% <100.00%> (ø)
eth/rpc/rpcapi/eth_api.go 42.10% <100.00%> (+2.84%) ⬆️
eth/rpc/rpcapi/net_api.go 78.94% <100.00%> (+7.51%) ⬆️
... and 7 more

@onikonychev
Copy link
Contributor Author

PR without event changes has been isolated in #2045

@Unique-Divine Unique-Divine deleted the on/backend-tests branch September 21, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants