test(evm): more bank extension tests and EVM ABCI integration tests to prevent regressions #2122
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (5)
WalkthroughThe pull request introduces several updates across multiple files, primarily focusing on enhancements to the Nibiru EVM. Key changes include updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip You can disable the changed files summary in the walkthrough.Disable the 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 (
|
| @@ -269,7 +271,7 @@ func (k *Keeper) ApplyEvmMsg(ctx sdk.Context, | |||
| ) | |||
|
|
|||
| defer func() { | |||
| if commit && err == nil && resp != nil && !resp.Failed() { | |||
There was a problem hiding this comment.
It's perfectly valid to have err == nil while the vmError is not empty. That will still result in a valid transaction.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2122 +/- ##
==========================================
+ Coverage 64.79% 64.92% +0.12%
==========================================
Files 273 273
Lines 21636 21656 +20
==========================================
+ Hits 14020 14061 +41
+ Misses 6644 6620 -24
- Partials 972 975 +3
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
x/evm/precompile/funtoken.go (1)
208-208: Implement EVM event emissions for balance changesYou've added a TODO comment to emit EVM events for balance changes of the sender and recipient. Implementing these events will enhance transparency and allow clients to listen and react to these balance changes effectively. Would you like assistance in generating the code for these events, or should I open a new GitHub issue to track this task?
x/evm/evmtest/tx.go (1)
Line range hint
219-247: Consider consolidating error wrapping patterns.The error handling could be more consistent. Currently, there are two different error wrapping patterns:
- Line 240: Using
fmt.Errorf("error while transferring wei: %w", err)- Line 245: Using the same pattern but in a different block
Consider consolidating the error handling into a single pattern at the return:
func (tx TxTransferWei) Run() (evmResp *evm.MsgEthereumTxResponse, err error) { // ... existing code ... ethTxMsg, err := NewEthTxMsgFromTxData( deps, gethcore.LegacyTxType, innerTxData, deps.NewStateDB().GetNonce(ethAcc.EthAddr), &to, amountWei, gasLimit, accessList, ) - if err != nil { - return evmResp, fmt.Errorf("error while transferring wei: %w", err) - } + if err != nil { + return nil, err + } evmResp, err = deps.App.EvmKeeper.EthereumTx(sdk.WrapSDKContext(deps.Ctx), ethTxMsg) - if err != nil { - err = fmt.Errorf("error while transferring wei: %w", err) - } - return evmResp, err + if err != nil { + return nil, fmt.Errorf("error while transferring wei: %w", err) + } + return evmResp, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
CHANGELOG.md(1 hunks)eth/rpc/backend/account_info.go(1 hunks)go.mod(1 hunks)x/common/testutil/testapp/testapp.go(2 hunks)x/evm/evmtest/tx.go(2 hunks)x/evm/evmtest/tx_test.go(1 hunks)x/evm/keeper/bank_extension.go(2 hunks)x/evm/keeper/bank_extension_test.go(2 hunks)x/evm/keeper/grpc_query_test.go(1 hunks)x/evm/keeper/keeper_test.go(2 hunks)x/evm/keeper/msg_ethereum_tx_test.go(1 hunks)x/evm/keeper/msg_server.go(6 hunks)x/evm/keeper/statedb_test.go(2 hunks)x/evm/precompile/funtoken.go(1 hunks)x/evm/precompile/funtoken_test.go(2 hunks)x/evm/precompile/wasm.go(1 hunks)x/evm/statedb/journal_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/evm/precompile/wasm.go
👮 Files not reviewed due to content moderation or server errors (3)
- x/evm/precompile/funtoken_test.go
- x/evm/keeper/msg_server.go
- x/evm/keeper/grpc_query_test.go
🔇 Additional comments (14)
x/evm/keeper/keeper_test.go (1)
Line range hint 6-23: LGTM!
The addition of logging in SetupTest enhances test traceability, and the implementation of the SetupTestSuite interface ensures proper setup for each test.
x/evm/evmtest/tx_test.go (1)
80-86: Refactoring enhances code maintainability
The refactoring to use the TxTransferWei struct with a Run method improves code organization and encapsulates the transaction logic, making it more maintainable and consistent with other test implementations.
x/evm/keeper/statedb_test.go (2)
48-52: Refactoring improves code clarity
The change to use the TxTransferWei struct for the transfer operation enhances code clarity by encapsulating parameters and logic within a structured approach.
65-69: Verify the correctness of the expected error message
In the test case where an insufficient wei amount is transferred, the expected error message is "wei amount is too small". Please verify that this error message matches the actual error produced by the TxTransferWei implementation.
You can confirm the error message by searching the codebase:
eth/rpc/backend/account_info.go (1)
86-90: LGTM! Improved code readability.
The parameters for GetProof are now properly formatted across multiple lines, enhancing code readability while maintaining the same functionality.
x/evm/keeper/msg_ethereum_tx_test.go (1)
41-41: LGTM! Improved numeric literal readability.
The change standardizes the formatting of large numbers using underscores as thousand separators (1_000_000 instead of 1000_000), following Go's conventions for improved readability.
x/common/testutil/testapp/testapp.go (1)
187-198: LGTM! Well-structured test helper function.
The new FundFeeCollector function is:
- Well-documented
- Follows existing patterns for funding helper functions
- Properly utilizes app constants for token denomination
- Provides a convenient abstraction for a common test setup task
x/evm/keeper/bank_extension.go (2)
93-114: LGTM: Gas handling implementation ensures deterministic gas consumption.
The implementation correctly manages gas consumption by:
- Capturing the initial gas state
- Using a deferred function to restore gas state
- Applying zero-cost gas config for operations
- Properly documenting the gas meter requirements
126-134: LGTM: Zero-cost gas configuration aligns with deterministic gas usage goal.
The zeroCostGasConfig variable is properly defined with all costs set to 0, which is consistent with the goal of ensuring deterministic gas consumption during state operations.
x/evm/statedb/journal_test.go (1)
110-110: LGTM: Enhanced error reporting in test.
The change improves test debugging by including the journal state in the failure message, making it easier to diagnose test failures.
x/evm/evmtest/tx.go (1)
212-217: LGTM: Well-structured parameter encapsulation.
The TxTransferWei struct provides a clean encapsulation of transfer parameters with clear field names and types.
go.mod (1)
64-64: LGTM: zerolog dependency addition
The addition of zerolog v1.32.0 is appropriate for structured logging capabilities.
CHANGELOG.md (2)
49-49: LGTM: Test entry follows changelog format
The new entry "- #2122 - test(evm): more bank extension tests" follows the established format of:
- Using hyphen for list items
- Including PR link
- Using conventional commit type prefix
Line range hint 1-48: LGTM: Well-structured changelog header
The changelog header follows best practices:
- Clear format guidelines
- Explicit versioning scheme (Semantic Versioning)
- Proper section organization
- Markdown formatting
Unique-Divine
changed the title
Purpose / Abstract
EthereumTxabci.BeginBlock -> abci.DeliverTx -> abci.EndBlockand displays proper gas consumption. → In doing this, I found out that nothing in our current test suite prevents a regression if we subtract out the consume gas step after anEthereumTx.EthereumTx > ApplyEvmMsgunnecessarily. TheBaseAppalready uses a temporary context and passes it in forDeliverTx, so there's not need to worry about cleanup inApplyEvmMsg.NibiruBankKeeperto verify it's not going to bring about unexpected state changes