feat(evm): typed events for contract creation, contract execution and transfer #1971
Conversation
WalkthroughThe recent updates to the NibiruChain project significantly enhance the Ethereum Virtual Machine (EVM) functionality by introducing typed events for contract interactions, improving event tracking and monitoring capabilities. Additionally, new structures and functions facilitate contract execution and testing, while existing tests have been refined for better clarity and robustness. Overall, these changes streamline event management and transaction handling, enhancing usability and performance. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EVM
participant EventEmitter
User->>EVM: Create/Execute Contract
EVM->>EventEmitter: Emit EventContractDeployed (for creation)
EVM->>EventEmitter: Emit EventContractExecuted (for execution)
EVM->>EventEmitter: Emit EventTransfer (if transfer occurs)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1971 +/- ##
==========================================
+ Coverage 65.95% 66.00% +0.05%
==========================================
Files 258 258
Lines 16384 16432 +48
==========================================
+ Hits 10806 10846 +40
Misses 4772 4772
- Partials 806 814 +8
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/evm/events.pb.gois excluded by!**/*.pb.go
Files selected for processing (8)
- CHANGELOG.md (1 hunks)
- proto/eth/evm/v1/events.proto (1 hunks)
- x/evm/evmtest/smart_contract.go (3 hunks)
- x/evm/evmtest/smart_contract_test.go (2 hunks)
- x/evm/evmtest/tx.go (2 hunks)
- x/evm/keeper/grpc_query_test.go (1 hunks)
- x/evm/keeper/msg_ethereum_tx_test.go (5 hunks)
- x/evm/keeper/msg_server.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/evm/keeper/grpc_query_test.go
Additional context used
GitHub Check: codecov/patch
x/evm/evmtest/smart_contract.go
[warning] 53-53: x/evm/evmtest/smart_contract.go#L53
Added line #L53 was not covered by tests
[warning] 106-106: x/evm/evmtest/smart_contract.go#L106
Added line #L106 was not covered by testsx/evm/evmtest/tx.go
[warning] 77-79: x/evm/evmtest/tx.go#L77-L79
Added lines #L77 - L79 were not covered by tests
[warning] 86-88: x/evm/evmtest/tx.go#L86-L88
Added lines #L86 - L88 were not covered by tests
[warning] 94-96: x/evm/evmtest/tx.go#L94-L96
Added lines #L94 - L96 were not covered by tests
Additional comments not posted (11)
x/evm/evmtest/smart_contract_test.go (1)
52-68: Verify the function usage and add assertions.The test function
TestExecuteContractTxMsgis well-structured. Ensure that all function calls toExecuteContractTxMsgmatch the new signature.Additionally, consider adding assertions to validate the expected state after the transaction execution.
+ // Add assertions to validate the expected state after execution. + // Example: + // s.Equal(expectedValue, actualValue)Verification successful
Verify the function usage and add assertions.
The function calls to
ExecuteContractTxMsgmatch the new signature consistently across the codebase.
msg_ethereum_tx_test.gosmart_contract_test.goConsider adding assertions to validate the expected state after the transaction execution.
+ // Add assertions to validate the expected state after execution. + // Example: + // s.Equal(expectedValue, actualValue)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ExecuteContractTxMsg` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'ExecuteContractTxMsg'Length of output: 2173
proto/eth/evm/v1/events.proto (3)
70-74: LGTM!The
EventTransfermessage definition is correct and aligns with the intended functionality.
77-80: LGTM!The
EventContractDeployedmessage definition is correct and aligns with the intended functionality.
83-86: LGTM!The
EventContractExecutedmessage definition is correct and aligns with the intended functionality.x/evm/evmtest/smart_contract.go (1)
32-42: LGTM!The
ArgsExecuteContractstruct is well-defined and aligns with the intended functionality.x/evm/evmtest/tx.go (1)
62-64: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
NewEthTxMsgFromTxDatamatch the new signature.Verification successful
Verified: Function usage matches the new signature.
The function call to
NewEthTxMsgFromTxDatainmsg_ethereum_tx_test.gohas been correctly updated to match the new signature.
msg_ethereum_tx_test.go: Lines around the function call show all required parameters are included.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewEthTxMsgFromTxData` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'NewEthTxMsgFromTxData'Length of output: 969
Script:
#!/bin/bash # Description: Extract the complete function call to `NewEthTxMsgFromTxData` in `msg_ethereum_tx_test.go` to verify all parameters. # Extract lines around the function call to ensure we capture the complete call. rg --type go -A 10 -B 5 $'NewEthTxMsgFromTxData' x/evm/keeper/msg_ethereum_tx_test.goLength of output: 492
x/evm/keeper/msg_ethereum_tx_test.go (3)
33-41: LGTM! Improved gas limit handling and event assertions.The changes enhance the test's robustness and coverage.
121-186: LGTM! Comprehensive test for contract execution.The new test function enhances the test coverage for contract execution.
190-201: LGTM! Improved test case structure withtxType.The changes enhance readability and maintainability.
x/evm/keeper/msg_server.go (1)
167-184: LGTM! Enhanced event emission logic for different transaction types.The new event emission logic improves the overall event management within the application.
However, ensure that the new event types are correctly handled throughout the codebase.
Verification successful
LGTM! Enhanced event emission logic for different transaction types.
The new event emission logic improves the overall event management within the application. The new event types are correctly handled throughout the codebase, as evidenced by their definitions and corresponding test cases.
x/evm/events.pb.go: Definitions ofEventContractDeployed,EventContractExecuted, andEventTransfer.x/evm/keeper/msg_server.go: Emission of the new event types.x/evm/keeper/msg_ethereum_tx_test.go: Test cases verifying the presence of the new event types.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new event types in the codebase. # Test: Search for the new event types. Expect: Correct handling of new event types. rg --type go -A 5 $'EventContractDeployed|EventContractExecuted|EventTransfer'Length of output: 16605
CHANGELOG.md (1)
89-89: Changelog entry looks good.The new entry is correctly formatted and provides sufficient information about the feature enhancement.
| innerTx.To = to | ||
| innerTx.Value = value | ||
| innerTx.Gas = gas |
There was a problem hiding this comment.
Reminder: Add tests for the new parameters.
The static analysis tools have flagged missing test coverage for the added lines.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Also applies to: 86-88, 94-96
Tools
GitHub Check: codecov/patch
[warning] 77-79: x/evm/evmtest/tx.go#L77-L79
Added lines #L77 - L79 were not covered by tests
| testContract, err := embeds.SmartContract_TestERC20.Load() | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
LGTM! But add tests for error handling.
The modification to use dynamic bytecode enhances flexibility. However, the error handling for loading the bytecode is not covered by tests.
Do you want me to generate the unit testing code for the error handling or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 53-53: x/evm/evmtest/smart_contract.go#L53
Added line #L53 was not covered by tests
| func ExecuteContractTxMsg(args ArgsExecuteContract) (ethTxMsg *evm.MsgEthereumTx, err error) { | ||
| gasLimit := args.GasLimit | ||
| if gasLimit == nil { | ||
| gasLimit = new(big.Int).SetUint64(gethparams.TxGas) | ||
| } | ||
| gethTxExecuteContract := &gethcore.AccessListTx{ | ||
| GasPrice: args.GasPrice, | ||
| Gas: gasLimit.Uint64(), | ||
| To: args.ContractAddress, | ||
| Data: args.Data, | ||
| Nonce: args.Nonce, | ||
| } | ||
| ethTx := gethcore.NewTx(gethTxExecuteContract) | ||
| ethTxMsg = new(evm.MsgEthereumTx) | ||
| err = ethTxMsg.FromEthereumTx(ethTx) | ||
| if err != nil { | ||
| return ethTxMsg, err | ||
| } | ||
| fromAcc := args.EthAcc | ||
| ethTxMsg.From = fromAcc.EthAddr.Hex() | ||
|
|
||
| gethSigner := fromAcc.GethSigner(args.EthChainIDInt) | ||
| keyringSigner := fromAcc.KeyringSigner | ||
| return ethTxMsg, ethTxMsg.Sign(gethSigner, keyringSigner) |
There was a problem hiding this comment.
LGTM! But add tests for error handling.
The function ExecuteContractTxMsg is well-structured. However, the error handling for transaction creation is not covered by tests.
Do you want me to generate the unit testing code for the error handling or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 106-106: x/evm/evmtest/smart_contract.go#L106
Added line #L106 was not covered by tests
@Unique-Divine you probably didn't finish the review as the comment is invisible. |
x/evm/keeper/msg_ethereum_tx_test.go
Outdated
| // Event "EventContractDeployed" must present | ||
| var sdkEvents = deps.Ctx.EventManager().Events() | ||
| contractDeployedEventType := "eth.evm.v1.EventContractDeployed" | ||
| err = testutil.AssertEventPresent(sdkEvents, contractDeployedEventType) | ||
| s.Require().NoError(err) | ||
|
|
||
| var contractDeployedEvent sdk.Event | ||
| for _, abciEvent := range sdkEvents { | ||
| if abciEvent.Type == contractDeployedEventType { | ||
| contractDeployedEvent = abciEvent | ||
| } | ||
| } | ||
| for _, err = range []error{ | ||
| testutil.EventHasAttributeValue(contractDeployedEvent, "sender", ethAcc.EthAddr.String()), | ||
| testutil.EventHasAttributeValue(contractDeployedEvent, "contract_addr", resp.Logs[0].Address), | ||
| } { | ||
| s.Require().NoError(err) |
There was a problem hiding this comment.
You can probably simplify this with
func RequireContainsTypedEvent(t require.TestingT, ctx sdk.Context, event proto.Message)
That way you can use the struct directly and won't need to put the type URL
There was a problem hiding this comment.
What a handy method! Refactored everywhere (see commit) and added one forgotten event check.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- x/evm/keeper/erc20_test.go (4 hunks)
- x/evm/keeper/msg_ethereum_tx_test.go (5 hunks)
Additional comments not posted (8)
x/evm/keeper/msg_ethereum_tx_test.go (4)
67-75: Event Assertion EnhancementThe use of
testutil.RequireContainsTypedEventimproves the readability and maintainability of event assertions.
98-104: Proper Error Handling for Insufficient GasThe error handling for exceeding the gas limit is well-implemented, ensuring that the correct error is asserted.
161-169: Event Assertion EnhancementThe use of
testutil.RequireContainsTypedEventimproves the readability and maintainability of event assertions.
225-234: Event Assertion EnhancementThe use of
testutil.RequireContainsTypedEventimproves the readability and maintainability of event assertions.x/evm/keeper/erc20_test.go (4)
107-116: Event Assertion EnhancementThe use of
testutil.RequireContainsTypedEventimproves the readability and maintainability of event assertions.
248-258: Event Assertion EnhancementThe use of
testutil.RequireContainsTypedEventimproves the readability and maintainability of event assertions.
368-378: Event Assertion EnhancementThe use of
testutil.RequireContainsTypedEventimproves the readability and maintainability of event assertions.
Line range hint
1-1: Well-Structured ERC20 Operations TestsThe function is well-structured, with clear steps for setting up the test environment, performing operations, and asserting results.
Summary by CodeRabbit
New Features
EventTransfer,EventContractDeployed, andEventContractExecuted.Bug Fixes
Documentation
CHANGELOG.mdto reflect new features and enhancements.Refactor