fix(evm): Fix DynamicFeeTx gas cap parameters #2017
Conversation
WalkthroughThe changes encompass a comprehensive refactoring of error handling, gas fee calculations, and transaction structures within the Ethereum Virtual Machine (EVM) codebase. Key modifications include the standardization of error types, enhancements to transaction fee management, and the introduction of new utility functions for testing. Additionally, the documentation has been improved to clarify the purpose and functionality of various components, ensuring better maintainability and readability across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EVM
participant RPC
User->>RPC: Send transaction request
RPC->>EVM: Process transaction
EVM->>EVM: Validate transaction
EVM->>EVM: Calculate gas fees
EVM->>RPC: Return transaction result
RPC->>User: Deliver transaction response
Assessment against linked issues
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 (
|
commit e7ffdadf55aa1aecf19f2add744067cfcd57e083
Author: Unique-Divine <realuniquedivine@gmail.com>
Date: Sat Aug 24 22:30:00 2024 -0500
fix: more RPC tests and refactor more
commit e17c50ddffc6767679a67b92bd5dc6fe3eeae94f
Author: Unique-Divine <realuniquedivine@gmail.com>
Date: Fri Aug 23 22:46:46 2024 -0500
test(e2e): get tests passing with non-default fee cap
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2017 +/- ##
==========================================
+ Coverage 65.90% 66.23% +0.32%
==========================================
Files 263 265 +2
Lines 16562 16657 +95
==========================================
+ Hits 10916 11032 +116
+ Misses 4824 4797 -27
- Partials 822 828 +6
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Outside diff range, codebase verification and nitpick comments (5)
eth/rpc/rpcapi/net_api_test.go (1)
7-13: Review of TestNetNamespace function.The test function is well-structured and covers basic functionality checks for the
EthRpc_NETAPI. It would be beneficial to add more detailed assertions or error messages to aid in debugging when tests fail.The code changes are approved.
Consider enhancing the test with more detailed assertions or error messages.
app/evmante/evmante_setup_ctx.go (1)
8-8: Inconsistent error handling: Transition tosdkerrorsincomplete.The codebase shows inconsistency in error handling, with both
sdkerrorsanderrortypesbeing used. To achieve the intended standardization, update all instances oferrortypestosdkerrors.
- Files using
errortypes:
x/evm/tx_data_access_list.goeth/rpc/rpc.goeth/rpc/backend/client_test.goeth/rpc/backend/evm_query_client_test.gox/evm/msg.goeth/safe_math.gox/evm/keeper/gas_fees.gox/evm/codec.goeth/crypto/ethsecp256k1/ethsecp256k1.goeth/eip712/message.goeth/eip712/types.goeth/eip712/eip712_legacy.goeth/assert.goapp/server/config/server_config.goapp/ante/gas_wanted.goapp/ante/reject_ethereum_tx_msgs.goAnalysis chain
Approve changes in error handling and suggest consistency check.
The change in error handling from
errortypestosdkerrorsaims to standardize and improve maintainability. Verify that this new approach is consistently applied across the codebase.The changes are approved.
Also applies to: 34-34
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of error handling across the codebase. # Test: Search for the usage of `sdkerrors` and `errortypes`. Expect: Only `sdkerrors` should be used. rg --type go $'sdkerrors' rg --type go $'errortypes'Length of output: 17201
app/evmante/evmante_verify_eth_acc.go (1)
46-46: Incomplete refactoring tosdkerrors.The refactoring to replace
errortypeswithsdkerrorsis not fully implemented. The following files still contain references toerrortypes:
eth/safe_math.goeth/eip712/message.goeth/rpc/rpc.goeth/eip712/types.goeth/rpc/backend/evm_query_client_test.goeth/rpc/backend/client_test.goeth/eip712/eip712_legacy.goeth/crypto/ethsecp256k1/ethsecp256k1.goeth/assert.gox/evm/codec.gox/evm/tx_data_access_list.gox/evm/msg.gox/evm/keeper/gas_fees.goapp/server/config/server_config.goapp/ante/reject_ethereum_tx_msgs.goapp/ante/gas_wanted.goPlease ensure that all instances of
errortypesare replaced withsdkerrorsto maintain consistency across the codebase.Analysis chain
Refactor error handling to use
sdkerrors.The use of
sdkerrors.ErrUnknownRequestinstead oferrortypes.ErrUnknownRequestis consistent with the new import and improves error clarity.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the error handling change is consistent across the codebase. # Test: Search for the old error type usage. Expect: No occurrences. rg --type go -A 5 $'errortypes.ErrUnknownRequest'Length of output: 48
Script:
#!/bin/bash # Description: Verify that there are no remaining references to the old error handling package `errortypes`. # Test: Search for any occurrences of `errortypes` in the codebase. Expect: No occurrences. rg --type go 'errortypes'Length of output: 6423
Tools
GitHub Check: codecov/patch
[warning] 46-46: app/evmante/evmante_verify_eth_acc.go#L46
Added line #L46 was not covered by testsapp/evmante/evmante_can_transfer.go (1)
43-43: Test Coverage Missing for Error Handling ChangesThe error handling changes involving
sdkerrors.ErrUnknownRequestinevmante_can_transfer.goare not covered by existing tests. Consider adding test cases to ensure these changes are properly tested.
- File:
app/evmante/evmante_can_transfer.go- Line: 43
Analysis chain
Error handling updates approved.
Updating the error types to
sdkerrorsenhances consistency in error reporting. However, line #L65 was not covered by tests.Also applies to: 65-65, 92-92
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new error handling is covered by tests. # Test: Search for test coverage for line #L65. Expect: Coverage information. rg --type python -A 5 $'app/evmante/evmante_can_transfer.go#L65'Length of output: 94
Script:
#!/bin/bash # Description: Verify that the new error handling is covered by tests. # Search for references to `sdkerrors.ErrUnknownRequest` in Go test files. rg 'sdkerrors.ErrUnknownRequest' --type go -A 5Length of output: 3510
Script:
#!/bin/bash # Description: Search for test functions in Go files that might cover the functionality related to `sdkerrors.ErrUnknownRequest`. # Search for test functions in Go test files. rg 'func Test' --type go -A 3Length of output: 64177
x/common/testutil/testnetwork/validator_node.go (1)
87-94: Enhancements to theValidatorstruct.The addition of new fields related to Ethereum operations (
EthRpcQueryClient,EthRpcBackend, etc.) significantly enhances the struct's capabilities. These fields are crucial for interacting with Ethereum's RPC interfaces and managing transactions.The code changes are approved.
Consider adding detailed documentation for each new field to explain their roles and interactions within the system.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
e2e/evm/bun.lockbis excluded by!**/bun.lockbx/evm/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (50)
- CHANGELOG.md (1 hunks)
- app/evmante/evmante_can_transfer.go (4 hunks)
- app/evmante/evmante_emit_event.go (2 hunks)
- app/evmante/evmante_gas_consume.go (7 hunks)
- app/evmante/evmante_increment_sender_seq.go (4 hunks)
- app/evmante/evmante_mempool_fees.go (2 hunks)
- app/evmante/evmante_setup_ctx.go (2 hunks)
- app/evmante/evmante_sigverify.go (4 hunks)
- app/evmante/evmante_validate_basic.go (7 hunks)
- app/evmante/evmante_verify_eth_acc.go (5 hunks)
- app/server/config/server_config.go (4 hunks)
- app/server/start.go (14 hunks)
- e2e/evm/test/basic_queries.test.ts (1 hunks)
- e2e/evm/test/contract_infinite_loop_gas.test.ts (1 hunks)
- e2e/evm/test/contract_send_nibi.test.ts (1 hunks)
- e2e/evm/test/debug_queries.test.ts (1 hunks)
- e2e/evm/test/erc20.test.ts (1 hunks)
- e2e/evm/test/eth_queries.test.ts (11 hunks)
- e2e/evm/test/utils.ts (1 hunks)
- eth/rpc/backend/call_tx.go (2 hunks)
- eth/rpc/backend/call_tx_test.go (3 hunks)
- eth/rpc/backend/tx_info.go (2 hunks)
- eth/rpc/rpc.go (1 hunks)
- eth/rpc/rpcapi/eth_api_test.go (9 hunks)
- eth/rpc/rpcapi/net_api_test.go (1 hunks)
- proto/eth/evm/v1/tx.proto (4 hunks)
- x/common/testutil/testnetwork/network.go (3 hunks)
- x/common/testutil/testnetwork/start_node.go (1 hunks)
- x/common/testutil/testnetwork/util.go (2 hunks)
- x/common/testutil/testnetwork/validator_node.go (4 hunks)
- x/evm/const.go (1 hunks)
- x/evm/evmtest/evmante.go (2 hunks)
- x/evm/evmtest/tx.go (2 hunks)
- x/evm/json_tx_args_test.go (5 hunks)
- x/evm/keeper/gas_fees.go (4 hunks)
- x/evm/keeper/gas_fees_test.go (1 hunks)
- x/evm/keeper/keeper.go (1 hunks)
- x/evm/keeper/msg_server.go (5 hunks)
- x/evm/msg.go (1 hunks)
- x/evm/msg_test.go (1 hunks)
- x/evm/tx.go (3 hunks)
- x/evm/tx_data.go (5 hunks)
- x/evm/tx_data_access_list.go (4 hunks)
- x/evm/tx_data_access_list_test.go (2 hunks)
- x/evm/tx_data_dynamic_fee.go (1 hunks)
- x/evm/tx_data_dynamic_fee_test.go (1 hunks)
- x/evm/tx_data_legacy.go (4 hunks)
- x/evm/tx_data_legacy_test.go (15 hunks)
- x/evm/tx_test.go (3 hunks)
- x/evm/vmtracer.go (1 hunks)
Files not reviewed due to server errors (2)
- eth/rpc/backend/tx_info.go
- x/evm/tx_data_dynamic_fee_test.go
Files skipped from review due to trivial changes (4)
- eth/rpc/backend/call_tx_test.go
- eth/rpc/rpc.go
- x/evm/json_tx_args_test.go
- x/evm/keeper/msg_server.go
Additional context used
GitHub Check: codecov/patch
app/evmante/evmante_setup_ctx.go
[warning] 34-34: app/evmante/evmante_setup_ctx.go#L34
Added line #L34 was not covered by testsx/evm/evmtest/evmante.go
[warning] 28-28: x/evm/evmtest/evmante.go#L28
Added line #L28 was not covered by tests
[warning] 75-75: x/evm/evmtest/evmante.go#L75
Added line #L75 was not covered by testsapp/evmante/evmante_increment_sender_seq.go
[warning] 65-65: app/evmante/evmante_increment_sender_seq.go#L65
Added line #L65 was not covered by testsx/evm/tx.go
[warning] 42-42: x/evm/tx.go#L42
Added line #L42 was not covered by tests
[warning] 46-46: x/evm/tx.go#L46
Added line #L46 was not covered by testsapp/evmante/evmante_mempool_fees.go
[warning] 49-49: app/evmante/evmante_mempool_fees.go#L49
Added line #L49 was not covered by testsapp/evmante/evmante_verify_eth_acc.go
[warning] 46-46: app/evmante/evmante_verify_eth_acc.go#L46
Added line #L46 was not covered by testsapp/evmante/evmante_can_transfer.go
[warning] 65-65: app/evmante/evmante_can_transfer.go#L65
Added line #L65 was not covered by testsapp/evmante/evmante_validate_basic.go
[warning] 84-84: app/evmante/evmante_validate_basic.go#L84
Added line #L84 was not covered by testsx/common/testutil/testnetwork/start_node.go
[warning] 35-35: x/common/testutil/testnetwork/start_node.go#L35
Added line #L35 was not covered by tests
[warning] 40-40: x/common/testutil/testnetwork/start_node.go#L40
Added line #L40 was not covered by tests
[warning] 57-57: x/common/testutil/testnetwork/start_node.go#L57
Added line #L57 was not covered by tests
[warning] 61-61: x/common/testutil/testnetwork/start_node.go#L61
Added line #L61 was not covered by tests
[warning] 98-99: x/common/testutil/testnetwork/start_node.go#L98-L99
Added lines #L98 - L99 were not covered by tests
[warning] 109-109: x/common/testutil/testnetwork/start_node.go#L109
Added line #L109 was not covered by tests
[warning] 117-117: x/common/testutil/testnetwork/start_node.go#L117
Added line #L117 was not covered by tests
[warning] 127-127: x/common/testutil/testnetwork/start_node.go#L127
Added line #L127 was not covered by tests
[warning] 140-140: x/common/testutil/testnetwork/start_node.go#L140
Added line #L140 was not covered by tests
[warning] 146-146: x/common/testutil/testnetwork/start_node.go#L146
Added line #L146 was not covered by tests
[warning] 153-153: x/common/testutil/testnetwork/start_node.go#L153
Added line #L153 was not covered by testsapp/evmante/evmante_gas_consume.go
[warning] 80-80: app/evmante/evmante_gas_consume.go#L80
Added line #L80 was not covered by testsx/evm/keeper/gas_fees.go
[warning] 175-175: x/evm/keeper/gas_fees.go#L175
Added line #L175 was not covered by testsx/common/testutil/testnetwork/validator_node.go
[warning] 255-261: x/common/testutil/testnetwork/validator_node.go#L255-L261
Added lines #L255 - L261 were not covered by tests
[warning] 263-266: x/common/testutil/testnetwork/validator_node.go#L263-L266
Added lines #L263 - L266 were not covered by testsx/evm/evmtest/tx.go
[warning] 281-292: x/evm/evmtest/tx.go#L281-L292
Added lines #L281 - L292 were not covered by testseth/rpc/backend/tx_info.go
[warning] 258-258: eth/rpc/backend/tx_info.go#L258
Added line #L258 was not covered by testsx/evm/msg.go
[warning] 287-287: x/evm/msg.go#L287
Added line #L287 was not covered by tests
[warning] 292-292: x/evm/msg.go#L292
Added line #L292 was not covered by testsx/common/testutil/testnetwork/network.go
[warning] 268-268: x/common/testutil/testnetwork/network.go#L268
Added line #L268 was not covered by tests
Additional comments not posted (127)
e2e/evm/test/contract_infinite_loop_gas.test.ts (1)
Line range hint
3-20: Review of Infinite loop gas contract test.The refactoring to use
deployContractInfiniteLoopGasimproves readability and maintainability. Ensure that this utility function correctly handles deployment and that the test accurately captures the expected out of gas error.The code changes are approved.
Consider adding additional checks or logs within the utility function to ensure it behaves as expected during deployment.
e2e/evm/test/erc20.test.ts (1)
Line range hint
4-20: Review of ERC-20 contract test.The refactoring to use
deployContractTestERC20potentially improves readability and maintainability. Ensure that this utility function correctly handles deployment and that the test accurately verifies the contract's functionality, especially the token transfer and balance verification.The code changes are approved.
Consider adding additional checks or logs within the utility function to ensure it behaves as expected during deployment.
x/evm/tx_data_access_list_test.go (2)
11-11: Verify the context and methods provided by theSuitetype.The change in receiver type from
*TxDataTestSuiteto*Suitesuggests a refactoring of the test suite structure. Ensure that theSuitetype is properly defined and provides all necessary methods and context required by the test functions.Verification successful
The
Suitetype provides the necessary context and methods.The
Suitetype inx/evm/tx_test.goincludes relevant fields and embedssuite.Suite, indicating it is well-suited for the test functionTestTestNewAccessList. The refactoring to use this type is consistent with a broader test suite organization. No issues found with the change in receiver type.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and context of the `Suite` type. # Test: Search for the definition of `Suite` type. Expect: Proper definition and context. rg --type go -A 10 $'type Suite struct'Length of output: 4514
35-35: Verify the context and methods provided by theSuitetype.The change in receiver type from
*TxDataTestSuiteto*Suitesuggests a refactoring of the test suite structure. Ensure that theSuitetype is properly defined and provides all necessary methods and context required by the test functions.Verification successful
The
Suitestruct is part of a shared test utility.The
Suitestruct used inx/evm/tx_data_access_list_test.gois likely defined in a shared location, embeddingsuite.Suitefrom thetestifypackage. This setup provides the necessary context and methods for the test functions, aligning with the refactoring pattern observed across the codebase. No further action is needed regarding the receiver type change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and context of the `Suite` type. # Test: Search for the definition of `Suite` type. Expect: Proper definition and context. rg --type go -A 10 $'type Suite struct'Length of output: 4514
Script:
#!/bin/bash # Description: Search for the `Suite` struct definition or its usage in `x/evm/tx_data_access_list_test.go`. # Test: Search for the `Suite` struct or its usage in the specific test file. rg --type go 'Suite' x/evm/tx_data_access_list_test.goLength of output: 154
Script:
#!/bin/bash # Description: Search for import statements or shared definitions related to the `Suite` struct. # Test: Search for import statements in the specific test file. rg --type go '^import' -A 10 x/evm/tx_data_access_list_test.goLength of output: 347
x/evm/tx_test.go (1)
17-17: Address potential loss of test coverage and verify the new test suite structure.The renaming of the test suite from
TxDataTestSuitetoSuiteand the removal of substantial test cases related toDynamicFeeTxcould lead to gaps in test coverage. Ensure that the newSuitetype is properly defined and that the remaining tests adequately cover the necessary functionality.Consider reintroducing or replacing the removed
DynamicFeeTxtests to maintain robust test coverage.Also applies to: 35-35, 54-54
e2e/evm/test/utils.ts (8)
9-9: Export ofaliceapproved.This change enhances modularity by allowing other modules to access
alice.
11-13: Utility functionhexifyapproved.This function correctly converts a number to its hexadecimal string representation, enhancing the utility of the module.
16-17: Export ofTENPOW12approved.This constant is useful for other components that require this specific value, enhancing the module's utility.
18-19: Export ofINTRINSIC_TX_GASapproved.This constant sets a baseline for transaction gas costs, which is a common practice in blockchain development.
20-26: Refactoring ofdeployContractTestERC20approved.Including a
maxFeePerGasparameter set toTENPOW12aligns with the PR's objectives to manage gas fees more effectively.
27-33: Refactoring ofdeployContractSendNibiapproved.Consistent application of the
maxFeePerGasparameter ensures uniformity in gas fee management.
34-39: Addition ofdeployContractInfiniteLoopGasapproved.This function consistently applies the
maxFeePerGasparameter, further strengthening the module's capabilities in handling contract deployments.
41-51: Update tosendTestNibiapproved.Including the
maxFeePerGasparameter ensures that the transaction adheres to the new gas fee structure.app/evmante/evmante_sigverify.go (3)
9-9: Approved import change.The replacement of
errortypeswithsdkerrorsaligns with the PR's objective to standardize error handling.
46-46: Approved error handling change.Using
sdkerrors.ErrUnknownRequestensures consistency with the Cosmos SDK's conventions.
55-55: Approved error handling changes.Using
sdkerrors.ErrNotSupportedandsdkerrors.ErrorInvalidSigneraligns with the standardized error handling approach.Also applies to: 64-64
app/evmante/evmante_increment_sender_seq.go (3)
7-7: Approved import change.The replacement of
errortypeswithsdkerrorsaligns with the PR's objective to standardize error handling.
41-41: Approved error handling change.Using
sdkerrors.ErrUnknownRequestensures consistency with the Cosmos SDK's conventions.
55-55: Approved error handling changes.Using
sdkerrors.ErrUnknownAddressandsdkerrors.ErrInvalidSequencealigns with the standardized error handling approach.Also applies to: 65-65
e2e/evm/test/debug_queries.test.ts (1)
7-11: Approved type safety changes.Explicit typing for
contractAddress,txHash,txIndex,blockNumber,blockHash, andreceiptimproves clarity and ensures conformity to expected types.Also applies to: 22-22
e2e/evm/test/contract_send_nibi.test.ts (2)
17-46: Review: New helper functiontestSendNibiThe function
testSendNibiis well-structured and effectively reduces code duplication by consolidating the logic for sending NIBI via different methods. However, the calculation of transaction costs (txCostMicronibiandtxCostWei) should be verified for correctness to ensure that the test accurately reflects the expected outcomes.The structure and modularity of the function are approved.
Please verify the correctness of the transaction cost calculations.
48-75: Review: Overall structure of the test suiteThe refactoring of the test suite to use the
testSendNibifunction in each test case has streamlined the process and improved maintainability. The explicit definition of timeouts for each test case is a good practice that ensures consistency in test execution.The overall structure and functionality of the test suite are approved.
app/evmante/evmante_verify_eth_acc.go (3)
7-7: Approved import change.The change from
errortypestosdkerrorsaligns with the PR's goal of standardizing error handling.
57-57: Refactor error handling to usesdkerrors.Updating to
sdkerrors.ErrInvalidAddressenhances consistency in error handling.
69-69: Refactor error handling to usesdkerrors.The change to
sdkerrors.ErrInvalidTypefor non-EOA addresses is appropriate and aligns with the new error handling strategy.x/evm/keeper/gas_fees_test.go (2)
16-114: Well-structured test function.The
TestVerifyFeefunction is well-organized, using a loop to iterate through test cases and thes.Runmethod for execution. This structure enhances readability and maintainability.
29-98: Comprehensive test cases.The test cases within
TestVerifyFeecover a variety of scenarios, including edge cases like gas limits and fee caps. Each case is clearly defined with expected outcomes, enhancing the robustness of the fee verification logic.app/evmante/evmante_validate_basic.go (1)
Line range hint
10-146: Consistent and clear error handling.The
AnteHandlefunction effectively handles basic validations for Ethereum transactions, using thesdkerrorspackage for error handling. This enhances clarity and aligns with Cosmos SDK conventions.Tools
GitHub Check: codecov/patch
[warning] 84-84: app/evmante/evmante_validate_basic.go#L84
Added line #L84 was not covered by testsx/evm/keeper/keeper.go (1)
123-128: Improved clarity and future-proofing inGetBaseFee.The changes to the
GetBaseFeefunction, including the use of an underscore for the unusedctxparameter and the shift to a constant value, enhance clarity and allow for potential future enhancements.x/evm/const.go (1)
13-15: Approved: Addition of BASE_FEE_MICRONIBI constant.The introduction of the
BASE_FEE_MICRONIBIconstant is a positive change, ensuring a standardized base fee across the codebase. This constant is correctly defined as (10^{12}) wei, aligning with the PR's objectives to handle gas fee calculations more effectively.x/evm/tx_data_legacy.go (1)
87-87: Approved: Method Renaming and Validation Logic Refinement.The renaming of methods to include "Wei" and the refinement of validation logic are significant improvements. These changes enhance clarity regarding the units of measurement and streamline the validation process, making the code more maintainable and easier to understand.
Also applies to: 95-97, 101-103, 107-108, 193-195, 198-200, 204-206
x/common/testutil/testnetwork/util.go (3)
Line range hint
21-45: Well-structured function for genesis file generation.The function
collectGenFilesis well-implemented with clear error handling and proper use of thegenutilpackage for app state generation and genesis file export.
Line range hint
47-76: Proper initialization of genesis files.The function
initGenFileseffectively initializes genesis files with accounts and balances, handling JSON operations and file writing with appropriate error checks.
Line range hint
78-104: Effective wallet filling function for testing.The function
FillWalletFromValidatoris well-implemented for testing purposes, facilitating transactions from a validator to a specified address with robust error handling and transaction response checks.app/evmante/evmante_gas_consume.go (2)
160-162: Improved readability in fee deduction function.The
deductFeefunction's signature formatting change enhances readability without altering the underlying logic.
Line range hint
9-137: Enhanced clarity and functionality in gas consumption validation.The
AnteHandlefunction is well-structured with improved variable naming (baseFeeMicronibiPerGas) and streamlined comments, enhancing both readability and maintainability. However, the new error handling logic introduced at line 80 needs test coverage.Verification successful
Test Coverage Verified for
AnteHandleFunctionThe
AnteHandlefunction is covered by tests, as evidenced by its presence in theapp/evmante/evmante_gas_consume_test.gofile. This suggests that the new error handling logic is adequately tested. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for new error handling logic in `AnteHandle`. # Test: Search for test cases covering line 80. Expect: At least one test case. rg --type python -A 5 $'AnteHandle'Length of output: 65
Script:
#!/bin/bash # Description: Verify test coverage for the `AnteHandle` function in Go test files. # Search for references to `AnteHandle` in Go test files. rg 'AnteHandle' --type go -A 5Length of output: 28094
Tools
GitHub Check: codecov/patch
[warning] 80-80: app/evmante/evmante_gas_consume.go#L80
Added line #L80 was not covered by testsx/evm/keeper/gas_fees.go (3)
25-30: Improved readability in intrinsic gas calculation function.The
GetEthIntrinsicGasfunction's signature reformatting enhances readability, making the function easier to understand without altering its logic.
37-46: Enhanced clarity in gas refund function.The
RefundGasfunction's comments and signature reformatting improve readability, making the refund process clearer without altering the underlying logic.
Line range hint
138-195: Clarified fee verification with improved parameter naming.The
VerifyFeefunction's renaming of thebaseFeeparameter tobaseFeeMicronibienhances clarity in gas fee calculations. However, the new parameter handling introduced at line 175 needs test coverage.Verification successful
Test coverage for
VerifyFeefunction is adequate.The
TestVerifyFeefunction inx/evm/keeper/gas_fees_test.goconfirms that the new parameter handling forbaseFeeMicronibiis covered by existing tests. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for new parameter handling in `VerifyFee`. # Test: Search for test cases covering line 175. Expect: At least one test case. rg --type python -A 5 $'VerifyFee'Length of output: 64
Script:
#!/bin/bash # Description: Verify test coverage for new parameter handling in `VerifyFee`. # Test: Search for test cases covering the `VerifyFee` function across all file types. rg 'VerifyFee' -A 5Length of output: 1955
Tools
GitHub Check: codecov/patch
[warning] 175-175: x/evm/keeper/gas_fees.go#L175
Added line #L175 was not covered by testsx/evm/tx_data.go (9)
50-61: Approved: MethodGetGasTipCapWeienhances clarity.The renaming of
GetGasTipCaptoGetGasTipCapWeiand the updated documentation provide clearer semantics for gas pricing, aligning with Ethereum's terminology.
63-74: Approved: MethodGetGasFeeCapWeienhances clarity.The renaming of
GetGasFeeCaptoGetGasFeeCapWeiand the detailed documentation clarify the functionality and align with Ethereum standards.
76-77: Approved: MethodGetValueWeienhances clarity.The renaming of
GetValuetoGetValueWeiprovides clearer semantics for transaction values, focusing on wei as the unit of measurement.
94-96: Approved: Methods for calculating effective fees and costs enhance clarity.The renaming of
EffectiveGasPriceandEffectiveFeeto include 'Wei' in their names reinforces the focus on wei as the unit of measurement, enhancing clarity. The methodEffectiveCostremains appropriately named since it inherently deals with costs in wei.
162-168: Approved: FunctionpriceTimesGascorrectly implements fee calculation.The renaming of
feetopriceTimesGasand its implementation clearly describe its functionality, which calculates the fee in wei for a given gas price and gas amount.
182-191: Approved: FunctionValidateTxDataAmountcorrectly validates transaction amounts.This new function appropriately checks if the transaction amount is non-negative and within valid bounds, using clear error handling and validation checks.
194-201: Approved: FunctionValidateTxDataTocorrectly validates the 'to' address.This new function appropriately checks if the 'to' address in a transaction is valid, using clear error handling and validation checks.
204-215: Approved: FunctionValidateTxDataGasPricecorrectly validates gas prices.This new function appropriately checks if the gas price in a transaction is non-negative and within valid bounds, using clear error handling and validation checks.
219-228: Approved: FunctionValidateTxDataChainIDcorrectly validates chain IDs.This new function appropriately checks if the chain ID in a transaction is derived from the transaction data, using clear error handling and validation checks.
x/evm/tx_data_legacy_test.go (19)
13-13: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
43-43: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
50-50: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
58-58: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
65-65: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
72-72: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
92-92: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
114-114: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
143-143: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
165-165: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
187-187: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
216-216: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
237-237: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
266-266: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
273-273: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
299-299: Test suite renaming and function pointer usage approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests. The use of a function pointer enhances the flexibility of the test cases.
357-357: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
381-381: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.
406-406: Test suite renaming approved.The renaming of the test suite from
TxDataTestSuitetoSuiteis consistent and does not affect the functionality of the tests.proto/eth/evm/v1/tx.proto (3)
77-91: Documentation for ECDSA fields in LegacyTx is clear and informative.The added comments provide essential insights into the ECDSA signature fields, which are crucial for understanding the security and integrity of transactions.
The documentation changes are approved.
124-138: Consistent ECDSA field documentation in AccessListTx enhances understanding.Repeating the detailed explanations for
v,r, andsfields across different transaction types ensures that developers have a consistent understanding regardless of the transaction structure.The documentation changes are approved.
172-186: Thorough and consistent documentation for ECDSA fields in DynamicFeeTx.The explanations for
v,r, andsfields are consistent with other transaction types, which helps in maintaining a uniform understanding across the board.The documentation changes are approved.
eth/rpc/backend/call_tx.go (2)
151-151: Useful addition of debug logging for transaction hash in SendRawTransaction.The new debug logging statement provides valuable information for monitoring and debugging transaction processing.
The logging change is approved.
295-297: Improved readability with reformatted EstimateGas method signature.The method signature reformatting enhances clarity and maintainability without altering functionality.
The formatting change is approved.
eth/rpc/rpcapi/eth_api_test.go (4)
275-389: Substantial improvements in Test_SmartContract method.The changes to use
ethAPIfor sending raw transactions and the updated balance checks enhance the test's robustness and integration with the underlying blockchain infrastructure.The method modifications are approved.
Verify the new transaction handling and balance checks in the
Test_SmartContractmethod to ensure they are functioning as expected.Verification successful
Verification successful for transaction handling and balance checks in Test_SmartContract.
The
Test_SmartContractmethod effectively usesethAPI.SendRawTransactionwith proper error handling and state verification. The balance checks are integrated into the test setup and assertions, ensuring robust transaction handling and accurate balance verification.
- The method includes error checks after sending transactions.
- It verifies the transaction's pending status and ensures sufficient account funds.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new transaction handling and balance checks in the `Test_SmartContract` method. # Test: Search for the usage of `ethAPI.SendRawTransaction` and balance checks. Expect: Proper handling and accurate balance verification. rg --type go -A 5 $'ethAPI.SendRawTransaction'Length of output: 860
47-50: Enhancement of TestSuite struct with ethAPI field.The addition of the
ethAPIfield allows for more direct and effective testing of Ethereum RPC methods, enhancing the test suite's functionality.The struct modification is approved.
Check the usage of the
ethAPIfield in test methods to ensure it's being utilized effectively.Verification successful
Effective Utilization of ethAPI Field in Test Methods
The
ethAPIfield in theTestSuitestruct is actively used in multiple test methods, confirming its effective utilization for testing Ethereum RPC methods. This enhancement improves the test suite's capability to simulate real-world scenarios.
- Usage confirmed in methods like
SendRawTransaction,GetPendingTransactions,GetTransactionReceipt, andGetTransactionLogs.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `ethAPI` field in test methods. # Test: Search for the field usage. Expect: At least one usage in Ethereum RPC method tests. rg --type go -A 5 $'ethAPI'Length of output: 2878
76-78: Improved SetupSuite method with ethAPI initialization.The modification to initialize the
ethAPIfield enhances the test suite's interaction capabilities with Ethereum's RPC, ensuring more robust and realistic tests.The method modification is approved.
Check the initialization of the
ethAPIfield in theSetupSuitemethod to ensure it's done correctly.Verification successful
Initialization of
ethAPIfield is correctly implemented.The
ethAPIfield is properly initialized withs.val.EthRPC_ETH, ensuring the test suite can effectively interact with Ethereum's RPC. This change is correctly implemented and aligns with the intended improvements.
- The initialization logic is confirmed to be correct and part of the setup sequence.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct initialization of `ethAPI` field in the `SetupSuite` method. # Test: Search for the initialization logic. Expect: Proper assignment from the validator. rg --type go -A 5 $'ethAPI ='Length of output: 458
11-11: Addition of cosmossdk.io/math import enhances mathematical operations.The new import is appropriate for handling precise mathematical operations in tests, improving accuracy and reliability.
The import addition is approved.
Check the usage of
cosmossdk.io/mathin the test methods to ensure it's being utilized effectively.e2e/evm/test/eth_queries.test.ts (11)
74-74: Good use of constants to validate estimated gas.Using
INTRINSIC_TX_GASto validate the estimated gas ensures that the test is robust and checks against a known benchmark.
97-97: Proper validation of gas price using hexified values.The use of
hexifyto validate the gas price ensures that the value is checked in the correct format, enhancing the test's accuracy.
145-145: Correct usage of deployment utility and code verification.The test effectively uses
deployContractSendNibifor deploying the contract and correctly verifies the contract code, ensuring the deployment is successful and the code is as expected.
Line range hint
153-177: Comprehensive testing of filter changes post-transaction.The test effectively uses
deployContractTestERC20for deploying the ERC-20 contract, creates a filter, executes a transaction, and checks the filter changes, ensuring that the filter functionality is correctly implemented and working as expected.
Line range hint
182-202: Correct handling of skipped test for unimplemented method.The test for
eth_getFilterLogsis correctly marked as skipped, acknowledging that the method is not implemented. This is good practice to ensure clarity and maintainability of the test suite.
207-222: Correct handling of skipped test for unimplemented method.The test for
eth_getLogsis correctly marked as skipped, acknowledging that the method is not implemented. This is good practice to ensure clarity and maintainability of the test suite.
Line range hint
226-253: Effective testing of storage proof retrieval and validation.The test uses
deployContractTestERC20to deploy an ERC-20 contract and correctly retrieves and validates the storage proof, ensuring that the proof functionality is correctly implemented and working as expected.
307-312: Proper testing of storage retrieval.The test correctly retrieves storage from a contract address and checks if the value is defined, ensuring that the storage retrieval functionality is working as expected.
Line range hint
312-339: Comprehensive testing of transaction retrieval by block hash and index.The test effectively retrieves a transaction by block hash and index, and by block number and index, and correctly validates the transaction details, ensuring that the transaction retrieval functionality is correctly implemented and working as expected.
346-346: Effective testing of transaction retrieval by hash.The test correctly retrieves a transaction by hash and validates the transaction details, ensuring that the transaction retrieval functionality is correctly implemented and working as expected.
358-358: Comprehensive testing of transaction receipt retrieval.The test effectively retrieves a transaction receipt by hash and validates the receipt details, ensuring that the transaction receipt retrieval functionality is correctly implemented and working as expected.
e2e/evm/test/basic_queries.test.ts (9)
61-64: LGTM!The test case correctly checks if the accounts list is not empty.
77-92: LGTM!The test case correctly fetches and validates the fee history data structure.
100-103: LGTM!The test case correctly checks if the account balance is greater than zero.
105-121: LGTM!The test case correctly validates the consistency between fetching a block by number and by hash.
123-133: LGTM!The test case correctly checks if the transaction count for a block by hash is non-negative.
135-142: LGTM!The test case correctly checks if the transaction count for a block by number is non-negative.
144-149: LGTM!The test case correctly checks if the contract code is defined after deployment.
151-177: LGTM!The test case correctly checks the filter changes after executing a transaction.
224-253: LGTM!The test case correctly checks the proof of storage after deploying a contract.
x/common/testutil/testnetwork/network.go (2)
266-270: Enhancement: Improved logging configuration inNew.The logger initialization has been updated to use
serverCtxLoggerinstead ofloggerNoOp, reflecting a change in how logging is handled based on theEnableTMLoggingconfiguration. This change suggests a more consistent logging approach when the logging feature is enabled, which can help in debugging and monitoring the network behavior more effectively.Tools
GitHub Check: codecov/patch
[warning] 268-268: x/common/testutil/testnetwork/network.go#L268
Added line #L268 was not covered by tests
457-457: Verify: Node and server startup inNew.The function call to start nodes has been renamed from
startInProcesstostartNodeAndServers, indicating a potential change in the underlying implementation or behavior of how nodes are started within the network. It's crucial to verify that this change does not introduce any regressions or unexpected behaviors in the network startup process.app/server/start.go (3)
Line range hint
105-114: Enhancement: Updated function calls and error handling inStartCmd.The function signatures for
startStandAloneandstartInProcesshave been updated to reflect the new context type fromserver.Contexttosdkserver.Context. Additionally, the error handling mechanism has been adjusted to returnsdkserver.ErrorCodeinstead ofserver.ErrorCode, ensuring that the error types are consistent with the new package. These changes streamline the server's functionality and improve the clarity of the codebase.Also applies to: 118-147
162-173: Enhancement: Flag declarations update.Several flag declarations that were previously associated with the
serverpackage are now prefixed withsdkserver, ensuring consistency across the command-line interface. This change is crucial for maintaining uniformity in how configuration flags are managed and interpreted within the system.
595-610: New Feature: Implementation ofOpenEVMIndexer.The newly introduced
OpenEVMIndexerfunction enhances the modularity and clarity of the code by providing a dedicated function for opening the EVM indexer database. This function is well-structured and follows good coding practices by separating concerns and using descriptive logging.x/evm/msg_test.go (1)
264-264: Clarify error message: Approved with a suggestion.The change in the error message enhances clarity by specifying the issue directly related to the gas price. However, consider a slight grammatical adjustment for better readability:
- errMsg: "cannot be nil: invalid gas price" + errMsg: "Invalid gas price: cannot be nil"CHANGELOG.md (1)
115-115: Entry for PR #2017 is correctly documented.The CHANGELOG entry for PR #2017 is appropriately placed and formatted according to the project's standards.
app/evmante/evmante_emit_event.go (2)
9-9: Standardization of error handling approved.The change from
errortypestosdkerrorsaligns error handling with SDK standards.
39-39: Error handling update approved.Updating the error type to
sdkerrors.ErrUnknownRequestenhances consistency in error reporting.x/evm/evmtest/evmante.go (2)
75-75: Consistent gas price conversion to Wei approved.This change aligns with the previous function, ensuring consistency across the codebase. However, this line was not covered by tests.
Tools
GitHub Check: codecov/patch
[warning] 75-75: x/evm/evmtest/evmante.go#L75
Added line #L75 was not covered by tests
28-28: Gas price conversion to Wei approved.The change ensures that gas prices are accurately represented in Wei. However, this line was not covered by tests.
Verification successful
Gas price conversion to Wei is covered by tests.
The change to use
evm.NativeToWeiforGasPriceis adequately covered by tests, as evidenced by the references found in various test files. This ensures that the functionality is tested across different scenarios.
- References to
GasPriceandevm.NativeToWeiare present in test files such asx/evm/keeper/gas_fees_test.go,x/evm/tx_data_legacy_test.go, and others.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new gas price conversion is covered by tests. # Test: Search for test coverage for line #L28. Expect: Coverage information. rg --type python -A 5 $'x/evm/evmtest/evmante.go#L28'Length of output: 83
Script:
#!/bin/bash # Description: Verify that the new gas price conversion is covered by tests. # Search for references to `GasPrice` in Go test files. rg 'GasPrice' --type go -A 5 # Search for references to `evm.NativeToWei` in Go test files. rg 'evm\.NativeToWei' --type go -A 5Length of output: 82413
Tools
GitHub Check: codecov/patch
[warning] 28-28: x/evm/evmtest/evmante.go#L28
Added line #L28 was not covered by testsapp/evmante/evmante_can_transfer.go (1)
9-9: Standardization of error handling approved.The change from
errortypestosdkerrorsaligns error handling with SDK standards.x/evm/vmtracer.go (1)
33-42: Refactoring and logging changes inNewTracerfunction.The renaming of
preCompilestoprecompileAddrsimproves clarity, and the streamlined logic for obtaining active precompiles enhances readability. The change in logging output forTracerJSONfromos.Stderrtoos.Stdoutmay affect how logs are captured in different environments.The code changes are approved.
Please verify the impact of changing the logging output destination on the system's logging and monitoring setup.
x/evm/tx_data_access_list.go (7)
153-153: Documentation update approved forGetGasPrice.The updated documentation clarifies that the gas price is returned in wei, enhancing understanding and consistency.
161-173: Renaming and documentation enhancement approved forGetGasTipCapWei.Renaming the function to include "Wei" explicitly clarifies the unit of measurement. The expanded documentation, including references to external resources, provides valuable context and aids in understanding the function's role in transaction priority.
177-184: Consistent renaming and documentation update approved forGetGasFeeCapWei.The function's renaming to
GetGasFeeCapWeiand the updated documentation are consistent with the PR's objectives to enhance clarity and precision in gas-related functions.
188-189: Renaming approved forGetValueWei.The renaming to
GetValueWeialigns with the PR's goal to standardize naming conventions across the codebase, emphasizing the unit of measurement in transaction-related functions.
289-291: Renaming and functional adjustment approved forEffectiveGasPriceWei.The renaming to
EffectiveGasPriceWeiand the adjustment to ensure the gas price does not fall below the base fee are critical for preventing transaction failures due to low gas prices. This change directly addresses issues highlighted in the PR.
294-296: Renaming approved forEffectiveFeeWei.The renaming to
EffectiveFeeWeienhances clarity regarding the unit of measurement, aligning with the PR's objectives to improve the semantic understanding of gas-related functions.
300-302: Renaming approved forEffectiveCost.The renaming to
EffectiveCost(with an emphasis on the unit wei) is consistent with the PR's theme of enhancing clarity and precision in naming transaction-related functions.x/evm/msg.go (2)
283-283: Approve the changes toGetEffectiveFee.The renaming of the parameter and method enhances clarity about the unit of measurement. Ensure that all references to this method are updated to pass the correct unit.
287-292: Approve the changes toGetEffectiveGasPrice.The renaming of the parameter and method enhances clarity about the unit of measurement. Ensure that all references to this method are updated to pass the correct unit.
Verification successful
Function usage is consistent with the new signature.
The function
GetEffectiveGasPriceis correctly used with the updated parameterbaseFeeWeiinapp/evmante/evmante_mempool_fees.go. Ensure that all other references, if any, are similarly updated.
- File:
app/evmante/evmante_mempool_fees.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetEffectiveGasPrice` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'GetEffectiveGasPrice'Length of output: 75
Script:
#!/bin/bash # Description: Verify all function calls to `GetEffectiveGasPrice` match the new signature. # Test: Search for the function usage in Go files. Expect: Only occurrences of the new signature. rg --type go -A 5 'GetEffectiveGasPrice'Length of output: 773
Tools
GitHub Check: codecov/patch
[warning] 287-287: x/evm/msg.go#L287
Added line #L287 was not covered by tests
[warning] 292-292: x/evm/msg.go#L292
Added line #L292 was not covered by testsx/evm/tx_data_dynamic_fee.go (8)
17-31: Review:BigIntMaxfunction implementation.The
BigIntMaxfunction correctly handles various cases ofnilinputs and compares twobig.Intvalues to return the maximum. This function is essential for ensuring that calculations involving potentiallynilvalues do not result in runtime errors.The implementation is correct and handles all edge cases appropriately.
78-99: Review:TxTypeandCopymethods forDynamicFeeTx.
- The
TxTypemethod correctly returns the transaction type using a constant fromgethcore.- The
Copymethod provides a deep copy of theDynamicFeeTx, which is crucial for ensuring that modifications to the copy do not affect the original object. This method correctly handles all fields, including slices which are properly duplicated usingcommon.CopyBytes.Both methods are implemented correctly and follow best practices for their respective functionalities.
101-126: Review: Various getter methods forDynamicFeeTx.These methods (
GetChainID,GetAccessList,GetData,GetGas,GetGasPrice) provide safe access to the properties ofDynamicFeeTx. They handle potentialnilvalues correctly, which is crucial to prevent runtime panics.The getter methods are correctly implemented and follow best practices for safe access to potentially nil values.
128-168: Review: Gas-related methods inDynamicFeeTx.
GetGasTipCapWeiandGetGasFeeCapWeimethods correctly convert optionalGasTipCapandGasFeeCapfields tobig.Int. They handlenilvalues appropriately, returningnilwhen the fields are not set, which aligns with the expected behavior in Ethereum transactions.- The documentation links provided in the comments are helpful for understanding the context and usage of these methods.
The implementation of these methods is correct, and the documentation enhances maintainability and readability.
170-189: Review: Additional getter methods forDynamicFeeTx.
GetValueWeiandGetNoncemethods are straightforward and correctly implemented. They provide access to the transaction's value and nonce, respectively.GetTomethod converts the hexadecimal address string to acommon.Addresstype, handling the case where the address might be empty.These methods are implemented correctly and provide safe, reliable access to the transaction fields.
191-209: Review:AsEthereumDatamethod for convertingDynamicFeeTxto Ethereum transaction data.This method constructs an Ethereum transaction data structure from a
DynamicFeeTx. It correctly retrieves raw signature values and other transaction fields, ensuring that all data is appropriately converted and formatted.The method is correctly implemented and effectively bridges the custom transaction structure with the standard Ethereum transaction format.
211-231: Review:SetSignatureValuesmethod forDynamicFeeTx.This method sets the signature values (V, R, S) and the chain ID for a transaction. It handles
nilinputs appropriately by not modifying the fields in such cases. The conversion ofchainIDfrombig.Inttosdkmath.Intis done correctly.The method is robust and correctly implements the logic for setting signature values, with appropriate checks for
nilinputs.
284-312: Review: Transaction cost calculation methods (Fee,Cost,EffectiveGasPriceWei,EffectiveFeeWei,EffectiveCost).These methods calculate various transaction costs based on gas prices and limits. They correctly implement the logic as per EIP-1559, handling the minimum of the sum of base fee and tip cap versus the fee cap.
EffectiveGasPriceWeimethod correctly implements the logic to determine the effective gas price, considering the base fee and ensuring it does not fall below this base fee.EffectiveFeeWeiandEffectiveCostmethods extend this logic to compute the total effective fee and cost of the transaction.The methods are correctly implemented, following the specifications of EIP-1559 and ensuring accurate transaction cost calculations.
app/server/config/server_config.go (1)
119-120: Addition ofTracerOptsfield approved.The new
TracerOptsfield in theEVMConfigstruct enhances the configurability of the EVM's tracing capabilities, which is crucial for debugging complex smart contracts.
| tipPrice := txData.EffectiveGasPriceWei(baseFee) | ||
|
|
||
| // Return the min of the max possible priorty and the derived priority | ||
| priority = math.MaxInt64 | ||
| priorityBig := new(big.Int).Quo(tipPrice, DefaultPriorityReduction.BigInt()) | ||
| derivedPriority := new(big.Int).Quo(tipPrice, DefaultPriorityReduction.BigInt()) | ||
|
|
||
| // safety check | ||
| if priorityBig.IsInt64() { | ||
| priority = priorityBig.Int64() | ||
| // Overflow safety check | ||
| var priorityBigI64 int64 | ||
| if derivedPriority.IsInt64() { | ||
| priorityBigI64 = derivedPriority.Int64() | ||
| } else { | ||
| priorityBigI64 = priority | ||
| } | ||
|
|
||
| return priority | ||
| return min(priority, priorityBigI64) |
There was a problem hiding this comment.
Review: Modified GetTxPriority function
The modifications to the GetTxPriority function, including the use of EffectiveGasPriceWei and the adjusted priority calculation logic, are well-thought-out and aim to improve the accuracy of transaction priority calculation. However, the added lines are not covered by tests, which is crucial for ensuring the reliability of the new logic.
The changes to the function are approved.
Please add tests to cover the new logic in the GetTxPriority function.
Tools
GitHub Check: codecov/patch
[warning] 42-42: x/evm/tx.go#L42
Added line #L42 was not covered by tests
[warning] 46-46: x/evm/tx.go#L46
Added line #L46 was not covered by tests
Review: Removal of DynamicFeeTx methods
The removal of several methods related to the DynamicFeeTx struct suggests a significant shift in how dynamic fee transactions are handled. It is important to verify the impact of these changes on the overall transaction processing within the Ethereum framework.
Please verify the impact of the removal of DynamicFeeTx methods on transaction processing.
Tools
GitHub Check: codecov/patch
[warning] 42-42: x/evm/tx.go#L42
Added line #L42 was not covered by tests
[warning] 46-46: x/evm/tx.go#L46
Added line #L46 was not covered by tests
| baseFeeMicronibi := d.evmKeeper.GetBaseFee(ctx) | ||
| baseFeeDec := math.LegacyNewDecFromBigInt(baseFeeMicronibi) | ||
|
|
||
| // if MinGasPrices is not set, skip the check | ||
| if minGasPrice.IsZero() { | ||
| return next(ctx, tx, simulate) | ||
| } else if minGasPrice.LT(baseFeeDec) { | ||
| minGasPrice = baseFeeDec | ||
| } | ||
|
|
||
| baseFee := d.evmKeeper.GetBaseFee(ctx) | ||
|
|
||
| for _, msg := range tx.GetMsgs() { | ||
| ethTx, ok := msg.(*evm.MsgEthereumTx) | ||
| if !ok { | ||
| return ctx, errors.Wrapf( | ||
| errortypes.ErrUnknownRequest, | ||
| sdkerrors.ErrUnknownRequest, | ||
| "invalid message type %T, expected %T", | ||
| msg, (*evm.MsgEthereumTx)(nil), | ||
| ) | ||
| } | ||
|
|
||
| effectiveGasPrice := ethTx.GetEffectiveGasPrice(baseFee) | ||
|
|
||
| if sdk.NewDecFromBigInt(effectiveGasPrice).LT(minGasPrice) { | ||
| baseFeeWei := evm.NativeToWei(baseFeeMicronibi) | ||
| effectiveGasPriceDec := math.LegacyNewDecFromBigInt( | ||
| evm.WeiToNative(ethTx.GetEffectiveGasPrice(baseFeeWei)), | ||
| ) | ||
| if effectiveGasPriceDec.LT(minGasPrice) { | ||
| // if sdk.NewDecFromBigInt(effectiveGasPrice).LT(minGasPrice) { | ||
| return ctx, errors.Wrapf( | ||
| errortypes.ErrInsufficientFee, | ||
| sdkerrors.ErrInsufficientFee, | ||
| "provided gas price < minimum local gas price (%s < %s). "+ | ||
| "Please increase the priority tip (for EIP-1559 txs) or the gas prices "+ | ||
| "(for access list or legacy txs)", | ||
| effectiveGasPrice.String(), minGasPrice.String(), | ||
| effectiveGasPriceDec, minGasPrice, |
There was a problem hiding this comment.
Review: Modified AnteHandle function
The modifications to the AnteHandle function, including the incorporation of a new base fee calculation and the adjustment of the minimum gas price, enhance the precision of gas price comparisons and improve the logic for determining gas prices in Ethereum transactions. However, the added line that adjusts the minimum gas price is not covered by tests, which is crucial for ensuring the reliability of the new logic.
The changes to the function are approved.
Please add tests to cover the new logic in the AnteHandle function.
Tools
GitHub Check: codecov/patch
[warning] 49-49: app/evmante/evmante_mempool_fees.go#L49
Added line #L49 was not covered by tests
| tmCfg.Instrumentation.Prometheus = false | ||
|
|
||
| if err := val.AppConfig.ValidateBasic(); err != nil { | ||
| return err |
There was a problem hiding this comment.
Add Test Coverage for Critical Setup Logic.
Several lines in the new file are not covered by tests, according to static analysis tools. It is crucial to add tests for these lines to ensure the reliability and correctness of the setup logic for nodes and servers within the test network.
Consider adding unit tests or integration tests that cover:
- Validation of configuration (
L35,L40). - Node construction and start logic (
L57,L61). - Error handling in asynchronous operations (
L98-L99). - gRPC server start logic (
L109,L117). - Context validation (
L127). - Indexer and JSON-RPC server setup (
L140,L146,L153).
This will help prevent potential runtime issues and ensure the stability of the test environment.
Also applies to: 40-40, 57-57, 61-61, 98-99, 109-109, 117-117, 127-127, 140-140, 146-146, 153-153
Tools
GitHub Check: codecov/patch
[warning] 35-35: x/common/testutil/testnetwork/start_node.go#L35
Added line #L35 was not covered by tests
| it("Simple transfer, balance check", async () => { | ||
| const amountToSend = toBigInt(5e12) * toBigInt(1e6) // unibi | ||
| const senderBalanceBefore = await provider.getBalance(account) | ||
| const recipientBalanceBefore = await provider.getBalance(alice) | ||
| expect(senderBalanceBefore).toBeGreaterThan(0) | ||
| expect(recipientBalanceBefore).toEqual(BigInt(0)) | ||
|
|
||
| const tenPow12 = toBigInt(1e12) | ||
|
|
||
| // Execute EVM transfer | ||
| const transaction: TransactionRequest = { | ||
| gasLimit: toBigInt(100e3), | ||
| to: alice, | ||
| value: amountToSend, | ||
| } | ||
| const txResponse = await account.sendTransaction(transaction) | ||
| await txResponse.wait(1, 10e3) | ||
| expect(txResponse).toHaveProperty("blockHash") | ||
|
|
||
| const senderBalanceAfter = await provider.getBalance(account) | ||
| const recipientBalanceAfter = await provider.getBalance(alice) | ||
|
|
||
| // Assert balances with logging | ||
| const gasUsed = 50000n // 50k gas for the transaction | ||
| const txCostMicronibi = amountToSend / tenPow12 + gasUsed | ||
| const txCostWei = txCostMicronibi * tenPow12 | ||
| const expectedSenderWei = senderBalanceBefore - txCostWei | ||
| console.debug("DEBUG should send via transfer method %o:", { | ||
| senderBalanceBefore, | ||
| amountToSend, | ||
| expectedSenderWei, | ||
| senderBalanceAfter, | ||
| }) | ||
| expect(senderBalanceAfter).toEqual(expectedSenderWei) | ||
| expect(recipientBalanceAfter).toEqual(amountToSend) | ||
| }) |
There was a problem hiding this comment.
Consider using a constant for the hardcoded gas value.
The test uses a hardcoded value of 50000n for the gas used in the transaction. It would be more maintainable and less error-prone to define this as a constant, possibly derived from the transaction response itself, to ensure it reflects the actual gas used.
| it("Simple transfer, balance check", async () => { | ||
| const amountToSend = toBigInt(5e12) * toBigInt(1e6) // unibi | ||
| const senderBalanceBefore = await provider.getBalance(account) | ||
| const recipientBalanceBefore = await provider.getBalance(alice) | ||
| expect(senderBalanceBefore).toBeGreaterThan(0) | ||
| expect(recipientBalanceBefore).toEqual(BigInt(0)) | ||
|
|
||
| const tenPow12 = toBigInt(1e12) | ||
|
|
||
| // Execute EVM transfer | ||
| const transaction: TransactionRequest = { | ||
| gasLimit: toBigInt(100e3), | ||
| to: alice, | ||
| value: amountToSend, | ||
| } | ||
| const txResponse = await account.sendTransaction(transaction) | ||
| await txResponse.wait(1, 10e3) | ||
| expect(txResponse).toHaveProperty("blockHash") | ||
|
|
||
| const senderBalanceAfter = await provider.getBalance(account) | ||
| const recipientBalanceAfter = await provider.getBalance(alice) | ||
|
|
||
| // Assert balances with logging | ||
| const gasUsed = 50000n // 50k gas for the transaction | ||
| const txCostMicronibi = amountToSend / tenPow12 + gasUsed | ||
| const txCostWei = txCostMicronibi * tenPow12 | ||
| const expectedSenderWei = senderBalanceBefore - txCostWei | ||
| console.debug("DEBUG should send via transfer method %o:", { | ||
| senderBalanceBefore, | ||
| amountToSend, | ||
| expectedSenderWei, | ||
| senderBalanceAfter, | ||
| }) | ||
| expect(senderBalanceAfter).toEqual(expectedSenderWei) | ||
| expect(recipientBalanceAfter).toEqual(amountToSend) | ||
| }) |
There was a problem hiding this comment.
Parameterize hardcoded values and verify gas calculation.
The test case uses hardcoded values for transaction amounts and gas. Consider parameterizing these to enhance flexibility and maintainability. Additionally, verify the correctness of the hardcoded gas used value (50000n) to ensure it aligns with actual transaction costs.
| func (val *Validator) AssertERC20Balance( | ||
| contract gethcommon.Address, | ||
| accAddr gethcommon.Address, | ||
| expectedBalance *big.Int, | ||
| s *suite.Suite, | ||
| ) { | ||
| input, err := embeds.SmartContract_ERC20Minter.ABI.Pack("balanceOf", accAddr) | ||
| s.NoError(err) | ||
| msg := geth.CallMsg{ | ||
| From: accAddr, | ||
| To: &contract, | ||
| Data: input, | ||
| } | ||
| recipientBalanceBeforeBytes, err := val.JSONRPCClient.CallContract(context.Background(), msg, nil) | ||
| s.NoError(err) | ||
| balance := new(big.Int).SetBytes(recipientBalanceBeforeBytes) | ||
| s.Equal(expectedBalance.String(), balance.String()) | ||
| } |
There was a problem hiding this comment.
New method AssertERC20Balance added to Validator.
This method enhances the Validator's capabilities by allowing it to assert ERC20 token balances using smart contract interactions. The implementation is robust, utilizing the Ethereum ABI and handling errors effectively.
The code changes are approved.
The new lines in this method are not covered by tests, which is crucial for ensuring the functionality works as expected.
Would you like me to help create unit tests for this method?
Tools
GitHub Check: codecov/patch
[warning] 255-261: x/common/testutil/testnetwork/validator_node.go#L255-L261
Added lines #L255 - L261 were not covered by tests
[warning] 263-266: x/common/testutil/testnetwork/validator_node.go#L263-L266
Added lines #L263 - L266 were not covered by tests
| func ValidLegacyTx() *evm.LegacyTx { | ||
| sdkInt := sdkmath.NewIntFromBigInt(evm.NativeToWei(big.NewInt(420))) | ||
| return &evm.LegacyTx{ | ||
| Nonce: 24, | ||
| GasLimit: 50_000, | ||
| To: gethcommon.HexToAddress("0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed").Hex(), | ||
| GasPrice: &sdkInt, | ||
| Amount: &sdkInt, | ||
| Data: []byte{}, | ||
| V: []byte{}, | ||
| R: []byte{}, | ||
| S: []byte{}, | ||
| } | ||
| } |
There was a problem hiding this comment.
New function ValidLegacyTx for testing legacy transactions.
The addition of ValidLegacyTx enhances the testing framework by providing a standardized legacy transaction object. This function is crucial for simplifying the creation of test cases across various scenarios.
The code changes are approved.
The new lines in this function are not covered by tests, which is essential for verifying its correctness and functionality.
Would you like me to help create unit tests for this function?
Tools
GitHub Check: codecov/patch
[warning] 281-292: x/evm/evmtest/tx.go#L281-L292
Added lines #L281 - L292 were not covered by tests
| func NewDynamicFeeTx(tx *gethcore.Transaction) (*DynamicFeeTx, error) { | ||
| txData := &DynamicFeeTx{ | ||
| Nonce: tx.Nonce(), | ||
| Data: tx.Data(), | ||
| GasLimit: tx.Gas(), | ||
| } | ||
|
|
||
| v, r, s := tx.RawSignatureValues() | ||
| if to := tx.To(); to != nil { | ||
| txData.To = to.Hex() | ||
| } | ||
|
|
||
| if tx.Value() != nil { | ||
| amountInt, err := eth.SafeNewIntFromBigInt(tx.Value()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| txData.Amount = &amountInt | ||
| } | ||
|
|
||
| if tx.GasFeeCap() != nil { | ||
| gasFeeCapInt, err := eth.SafeNewIntFromBigInt(tx.GasFeeCap()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| txData.GasFeeCap = &gasFeeCapInt | ||
| } | ||
|
|
||
| if tx.GasTipCap() != nil { | ||
| gasTipCapInt, err := eth.SafeNewIntFromBigInt(tx.GasTipCap()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| txData.GasTipCap = &gasTipCapInt | ||
| } | ||
|
|
||
| if tx.AccessList() != nil { | ||
| al := tx.AccessList() | ||
| txData.Accesses = NewAccessList(&al) | ||
| } | ||
|
|
||
| txData.SetSignatureValues(tx.ChainId(), v, r, s) | ||
| return txData, nil |
There was a problem hiding this comment.
Review: NewDynamicFeeTx constructor function.
This function constructs a new DynamicFeeTx from a given gethcore.Transaction. It properly handles potential nil values and errors from helper functions like eth.SafeNewIntFromBigInt. The use of pointers for optional fields (To, Amount, GasFeeCap, GasTipCap, Accesses) is consistent with Go's handling of optional or nullable values.
However, there's a potential improvement in error handling:
- When multiple fields could generate errors (like
Amount,GasFeeCap,GasTipCap), the function returns immediately on the first error, which is generally good. However, logging these errors or providing more context in the error messages could help with debugging and maintenance.
Consider enhancing error messages with more context or adding logging for error conditions to aid in troubleshooting.
| // Validate performs a stateless validation of the tx fields. | ||
| func (tx DynamicFeeTx) Validate() error { | ||
| if tx.GasTipCap == nil { | ||
| return errorsmod.Wrap(ErrInvalidGasCap, "gas tip cap cannot nil") | ||
| } | ||
|
|
||
| if tx.GasFeeCap == nil { | ||
| return errorsmod.Wrap(ErrInvalidGasCap, "gas fee cap cannot nil") | ||
| } | ||
|
|
||
| if tx.GasTipCap.IsNegative() { | ||
| return errorsmod.Wrapf(ErrInvalidGasCap, "gas tip cap cannot be negative %s", tx.GasTipCap) | ||
| } | ||
|
|
||
| if tx.GasFeeCap.IsNegative() { | ||
| return errorsmod.Wrapf(ErrInvalidGasCap, "gas fee cap cannot be negative %s", tx.GasFeeCap) | ||
| } | ||
|
|
||
| if !eth.IsValidInt256(tx.GetGasTipCapWei()) { | ||
| return errorsmod.Wrap(ErrInvalidGasCap, "out of bound") | ||
| } | ||
|
|
||
| if !eth.IsValidInt256(tx.GetGasFeeCapWei()) { | ||
| return errorsmod.Wrap(ErrInvalidGasCap, "out of bound") | ||
| } | ||
|
|
||
| if tx.GasFeeCap.LT(*tx.GasTipCap) { | ||
| return errorsmod.Wrapf( | ||
| ErrInvalidGasCap, "max priority fee per gas higher than max fee per gas (%s > %s)", | ||
| tx.GasTipCap, tx.GasFeeCap, | ||
| ) | ||
| } | ||
|
|
||
| if !eth.IsValidInt256(tx.Fee()) { | ||
| return errorsmod.Wrap(ErrInvalidGasFee, "out of bound") | ||
| } | ||
|
|
||
| for _, err := range []error{ | ||
| ValidateTxDataAmount(&tx), | ||
| ValidateTxDataTo(&tx), | ||
| ValidateTxDataChainID(&tx), | ||
| } { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Review: Validate method for DynamicFeeTx.
This method performs several checks to ensure the transaction fields are valid:
- Checks for
niland negative values inGasTipCapandGasFeeCap. - Validates that
GasFeeCapis not less thanGasTipCap. - Uses helper functions to validate other fields like
Amount,To, andChainID.
The method is comprehensive and covers all critical aspects of transaction validation. However, the error messages could be more descriptive to aid in debugging.
Enhance error messages with more detailed descriptions or include field values in the messages to improve clarity and maintainability.
| Tracer: DefaultEVMTracer, | ||
| TracerOpts: tracerslogger.Config{ | ||
| EnableMemory: false, // disable | ||
| DisableStack: false, // enable stack | ||
| DisableStorage: false, // enable storage | ||
| EnableReturnData: false, // disable | ||
| Debug: true, // enable debug | ||
| Limit: 0, | ||
| Overrides: nil, | ||
| }, |
There was a problem hiding this comment.
Approve the updated initialization of TracerOpts with a minor correction needed.
The initialization of TracerOpts with specific default values in the DefaultEVMConfig function is well-thought-out, optimizing the tracing output for better performance. However, there is a typo in the mapstructure tag for TracerOpts in the EVMConfig struct.
Please correct the typo in the mapstructure tag from mapstucture to mapstructure:
- TracerOpts tracerslogger.Config `mapstucture:"tracer_opts"`
+ TracerOpts tracerslogger.Config `mapstructure:"tracer_opts"`Committable suggestion was skipped due to low confidence.
Purpose / Abstract
Fixes bug related to the use of gas cap values.
DynamicFeeTxtestnetworkteststestnetwork.Validatorstruct so that they can be used in tests.DynamicFeeTxuser experience by automatically enforcing the base fee for low gas fee values.2024-08-25 - Corrected unit handling in the
app/evmantepackage.2024-08-28 - User experience improvement
I had a brief discussion with @onikonychev and the fact that ethers sets low, invalid values by default for gas tip and gas fee cap in the
DynamicFeeTxthat are below the minimum possible base fee causes a UX problem in that Ethereum developers would need to manually set the value for most transactions.The solution implemented here adds a slight change to the
EffectiveGasPrice,EffectiveFeeWei, andEffectiveCostfunctions onTxDatastruct, where the minimum possible value for the gas price or gas fee cap is taken to be the base fee. This prevents clients that have default values from broadcasting transactions with values between [0, 10^12 - 1]. This fixes the issue from ethers v5 raised on Slack.[Behavior with no changes to `TxData` implementations]
Ethers sets the gas fee cap to 2 (wei per unit gas) automatically when no argument is given. As a workaround to successfully send transactions in the test, an additional argument was added to each
TransactionRequest.This isn't the worst behavior in the world, but it's a quite unintuitive requirement to put on the developer. To improve upon this, we'll enforce the minimum possible valid values rather than raising errors. In other words, if the
maxFeePerGasis below the lowest valid value (baseFee[per gas]), then it will take thatbaseFeevalue instead.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests