chore(evm): Augment the Wasm msg handler so that wasm contracts cannot send MsgEthereumTx #2159
Conversation
…t send MsgEthereumTx
Unique-Divine
force-pushed
the
ud/ban-wasm-to-evm
branch
from
e607488 to
5ce2608
Compare
WalkthroughThis pull request introduces enhancements to the Wasm message handling mechanism in the Nibiru blockchain, specifically focusing on preventing Wasm contracts from sending Ethereum transactions. The changes span multiple files, including Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2159 +/- ##
==========================================
- Coverage 65.14% 64.96% -0.19%
==========================================
Files 277 277
Lines 22238 22347 +109
==========================================
+ Hits 14488 14518 +30
- Misses 6760 6836 +76
- Partials 990 993 +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: 4
🧹 Nitpick comments (3)
app/wasmext/wasm.go (2)
53-55: Generalize the message type check for unauthorized messagesCurrently, the code specifically checks for
MsgEthereumTxto prevent Wasm contracts from sending this message type. Consider generalizing this check to exclude all unauthorized messages by maintaining a list of allowed message types or implementing a more flexible permission system.Apply this diff to implement a whitelist approach:
func (h SDKMessageHandler) handleSdkMessage(ctx sdk.Context, contractAddr sdk.Address, msg sdk.Msg) (*sdk.Result, error) { if err := msg.ValidateBasic(); err != nil { return nil, err } + // Define allowed message types + allowedMsgTypes := map[string]bool{ + "/cosmos.bank.v1beta1.MsgSend": true, + // Add other allowed message types here + } + // make sure this account can send it for _, acct := range msg.GetSigners() { if !acct.Equals(contractAddr) { return nil, errors.Wrap(sdkerrors.ErrUnauthorized, "contract doesn't have permission") } } msgTypeUrl := sdk.MsgTypeURL(msg) - if msgTypeUrl == sdk.MsgTypeURL(new(evm.MsgEthereumTx)) { + if !allowedMsgTypes[msgTypeUrl] { return nil, errors.Wrap(sdkerrors.ErrUnauthorized, "unauthorized message type") }
121-122: Handle encoding errors gracefullyIn the
DispatchMsgfunction, ifh.encoders.Encodereturns an error, it propagates up without additional context. Consider wrapping this error to provide more informative feedback.Apply this diff to wrap the error:
if err != nil { - return nil, nil, err + return nil, nil, errors.Wrap(err, "failed to encode message") }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 121-122: app/wasmext/wasm.go#L121-L122
Added lines #L121 - L122 were not covered by testsapp/wasmext/wasmext_test.go (1)
72-72: Assert specific error types instead of error messagesRelying on exact error messages in tests can lead to fragile tests that break if the message changes. Instead, assert the error type to make the test more robust.
Apply this diff to check for the specific error type:
) - s.Require().ErrorContains(err, "Wasm VM to EVM call pattern is not yet supported") + s.Require().ErrorIs(err, sdkerrors.ErrUnauthorized)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)app/app.go(2 hunks)app/keepers.go(2 hunks)app/keepers/all_keepers.go(2 hunks)app/wasmext/stargate_query_test.go(1 hunks)app/wasmext/wasm.go(2 hunks)app/wasmext/wasmext_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/wasmext/wasm.go
[warning] 42-43: app/wasmext/wasm.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 48-49: app/wasmext/wasm.go#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 58-62: app/wasmext/wasm.go#L58-L62
Added lines #L58 - L62 were not covered by tests
[warning] 68-68: app/wasmext/wasm.go#L68
Added line #L68 was not covered by tests
[warning] 121-122: app/wasmext/wasm.go#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 129-135: app/wasmext/wasm.go#L129-L135
Added lines #L129 - L135 were not covered by tests
[warning] 137-137: app/wasmext/wasm.go#L137
Added line #L137 was not covered by tests
🔇 Additional comments (7)
app/wasmext/wasm.go (1)
46-50: Verify authorization logic for message signersThe current implementation checks if the contract address is among the signers of the message. Ensure that this check aligns with the desired security model. If the contract should be the sole signer, consider enforcing that explicitly.
Run the following script to identify messages where the contract address is not the sole signer:
✅ Verification successful
Authorization check is correctly implemented
The current implementation properly enforces that the contract must be the sole signer of the message. This is the correct security model for Wasm contract message execution as it:
- Prevents unauthorized message execution
- Ensures message authenticity
- Maintains proper authorization boundaries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find message types where the contract is not the only signer. # Test: Search for messages with multiple signers. Expect: No results if the contract should be the only signer. ast-grep --pattern $'sdk.Msg { GetSigners()[] { $_ } }' --json | jq '.[] | select(.match.captures[0].length > 1)'Length of output: 122
Script:
#!/bin/bash # Find GetSigners implementations and authorization checks echo "=== GetSigners implementations ===" rg "func.*GetSigners" -A 3 echo -e "\n=== Authorization patterns ===" rg "acct\.Equals.*contract" -B 2 -A 2 echo -e "\n=== Related tests ===" rg "TestGetSigners|TestAuthorization" --type goLength of output: 6801
Script:
#!/bin/bash # Find related wasm message handling code and tests echo "=== Wasm message handlers ===" rg "func.*Handle.*Msg" app/wasmext/ -A 5 echo -e "\n=== Wasm tests ===" fd -e go . app/wasmext/ --exec rg -l "func Test" echo -e "\n=== Contract authorization context ===" rg -B 3 -A 3 "contractAddr" app/wasmext/Length of output: 3108
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-49: app/wasmext/wasm.go#L48-L49
Added lines #L48 - L49 were not covered by testsapp/wasmext/wasmext_test.go (1)
28-31: Initialize Wasm Message Handler with dependenciesEnsure that all necessary dependencies are properly initialized when creating the
wasmMsgHandler. Missing or misconfigured dependencies might lead to unexpected errors during message dispatch.app/keepers/all_keepers.go (1)
72-72: Addition ofWasmMsgHandlerArgsenhances modularityAdding
WasmMsgHandlerArgsto thePublicKeepersstruct improves the modularity and configurability of the Wasm message handling. This change aligns with best practices for dependency injection.app/wasmext/stargate_query_test.go (1)
38-39: LGTM! Clean refactoring to use testify/suite.The conversion of the test function to a Suite method maintains the same test logic while improving the test structure.
app/app.go (1)
123-127: LGTM! Well-structured addition of Wasm message handler args.The addition of
wasmMsgHandlerArgsparameter enables customization of Wasm message handling, which is essential for preventing Wasm contracts from sending specific message types.Let's verify that all calls to
GetWasmOptshave been updated with the new parameter:Also applies to: 136-136
✅ Verification successful
✓ All calls to GetWasmOpts have been properly updated
The single call to GetWasmOpts in app/keepers.go correctly includes the new wasmMsgHandlerArgs parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to GetWasmOpts to ensure they include the new parameter rg "GetWasmOpts\(" -A 3Length of output: 387
app/keepers.go (2)
469-477: LGTM! Well-structured initialization of WasmMsgHandlerArgs.The initialization includes all necessary dependencies for proper message handling and routing.
486-491: LGTM! Consistent usage of WasmMsgHandlerArgs in WasmKeeper initialization.The change maintains consistency by using the same message handler args instance for both the WasmKeeper and GetWasmOpts.
Let's verify that all necessary interfaces are implemented by the provided dependencies:
Also applies to: 497-497
✅ Verification successful
Verified: Dependencies properly implement required interfaces
The dependencies passed to WasmKeeper are properly sourced from standard Cosmos SDK and IBC-Go implementations that satisfy the required interfaces. The same WasmMsgHandlerArgs instance is consistently used across both the WasmKeeper and GetWasmOpts initialization, maintaining proper dependency injection.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for interface definitions to verify compatibility ast-grep --pattern 'type $interface interface { $$$ }'Length of output: 58
Script:
#!/bin/bash # Search for interface definitions and implementations related to WasmMsgHandlerArgs rg -A 5 "type (Ics4Wrapper|ChannelKeeper|PortKeeper|CapabilityKeeper|PortSource|Router) interface" --type go # Search for WasmMsgHandlerArgs struct to understand required interfaces rg -A 10 "type WasmMsgHandlerArgs struct" --type goLength of output: 164
Script:
#!/bin/bash # Search for WasmKeeper initialization and usage rg -l "NewWasmKeeper|NewKeeper.*Wasm" --type go # Search for files containing these specific dependency names rg -l "Ics4Wrapper|ChannelKeeper|PortKeeper|CapabilityKeeper|PortSource" --type go # Look for go.mod to understand external dependencies cat go.modLength of output: 12288
Script:
#!/bin/bash # Check the implementation details cat app/keepers.go app/wasmext/wasm.go # Search for any compilation errors or interface checks rg -A 5 "var _ .*(Ics4Wrapper|ChannelKeeper|PortKeeper|CapabilityKeeper|PortSource)" --type goLength of output: 38790
| if err := msg.ValidateBasic(); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for validation error handling
The error handling when msg.ValidateBasic() fails in the handleSdkMessage function is not covered by tests. Consider adding a test case to ensure that messages failing basic validation are properly rejected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-43: app/wasmext/wasm.go#L42-L43
Added lines #L42 - L43 were not covered by tests
| if handler := h.router.Handler(msg); handler != nil { | ||
| // ADR 031 request type routing | ||
| msgResult, err := handler(ctx, msg) | ||
| return msgResult, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for message routing logic
The message handling within handleSdkMessage, especially the routing of messages using the handler, lacks test coverage. Adding tests to verify that messages are correctly routed and processed will improve reliability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-62: app/wasmext/wasm.go#L58-L62
Added lines #L58 - L62 were not covered by tests
| // proto messages and has registered all `Msg services`, then this | ||
| // path should never be called, because all those Msgs should be | ||
| // registered within the `msgServiceRouter` already. | ||
| return nil, errors.Wrapf(sdkerrors.ErrUnknownRequest, "can't route message %+v", msg) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test for unknown message types
The error path when a message cannot be routed (unknown request) is not covered by tests. Include a test case to ensure that appropriate errors are returned for unrecognized messages.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 68-68: app/wasmext/wasm.go#L68
Added line #L68 was not covered by tests
| data = append(data, res.Data) | ||
| // append events | ||
| sdkEvents := make([]sdk.Event, len(res.Events)) | ||
| for i := range res.Events { | ||
| sdkEvents[i] = sdk.Event(res.Events[i]) | ||
| } | ||
| events = append(events, sdkEvents...) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for event and data aggregation
The code segment that appends res.Data and processes events is not covered by tests. Ensure that tests cover the aggregation of data and events from dispatched messages.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-135: app/wasmext/wasm.go#L129-L135
Added lines #L129 - L135 were not covered by tests
|
|
||
| // WasmVM to EVM call pattern is not yet supported. This test verifies the | ||
| // Nibiru's [wasmkeeper.Option] function as expected. | ||
| func (s *Suite) TestEvmFilter() { |
There was a problem hiding this comment.
should we add here some happy paths too to ensure the functionality not regressed?
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…uChain/nibiru into refactor/evm/call-contract
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (24)
eth/rpc/backend/backend_suite_test.go (1)
180-180: Consider a shorter timeout duration.While increasing the timeout from 5 seconds to 5 minutes helps prevent flaky tests by allowing more time for transaction inclusion, 5 minutes might be excessive and could significantly slow down the test suite. Consider using a shorter duration (e.g., 30 seconds or 1 minute) that still provides enough buffer for network delays and block times.
- ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)x/evm/evmtest/evmante.go (2)
23-31: Consider using appropriate gas limit for transfers.While the code works, it's using
GasLimitCreateContract()for a simple transfer transaction. This is excessive as transfers typically require less gas than contract deployments.Consider creating a separate gas limit constant for transfers, which would be more representative of real-world scenarios:
- GasLimit: GasLimitCreateContract().Uint64(), + GasLimit: 21000, // Standard ETH transfer gas limit
70-78: Consider parameterizing the nonce value.The nonce is hardcoded to 1, which might not be suitable for all test scenarios, especially when testing multiple transactions from the same account.
Consider accepting the nonce as a parameter, similar to
HappyTransferTx:-func HappyCreateContractTx(deps *TestDeps) *evm.MsgEthereumTx { +func HappyCreateContractTx(deps *TestDeps, nonce uint64) *evm.MsgEthereumTx { evmTxArgs := &evm.EvmTxArgs{ ChainID: deps.App.EvmKeeper.EthChainID(deps.Ctx), - Nonce: 1, + Nonce: nonce,x/evm/precompile/wasm_test.go (4)
39-115: Consider adding test documentation.The test implementation is solid with proper error checking and verification. However, adding a brief comment explaining the test's purpose and the significance of the token operations would improve maintainability.
Add a doc comment like:
+// TestExecuteHappy verifies the successful execution of Wasm contract operations, +// specifically testing token creation and minting through the EVM precompile. func (s *WasmSuite) TestExecuteHappy() {
153-170: Enhance error messages for better debugging.While the implementation is solid, the error messages could be more descriptive to aid in debugging test failures.
Consider enhancing error messages:
-s.Require().NoError(err) +s.Require().NoError(err, "failed to pack contract input for counter state query") -s.Require().NotEmpty(ethTxResp.Ret) +s.Require().NotEmpty(ethTxResp.Ret, "empty response from counter state query")
330-345: Document test cases for better maintainability.The test implementation is solid, but adding documentation for each test case would improve maintainability.
Consider adding comments for test cases:
testcases := []struct { + // name of the test case name string + // method being tested methodName precompile.PrecompileMethod + // arguments to pass to the method callArgs []any + // expected error message wantError string }{
Line range hint
501-543: Consider adding explicit state change assertions.While the test effectively verifies atomic execution, adding explicit assertions about the expected state changes (or lack thereof) would make the test's intentions clearer.
Consider adding assertions:
// Verify that no state changes occurred test.AssertWasmCounterState(&s.Suite, deps, evmObj, wasmContract, 0) +// Verify that the first message was not executed +s.Require().Equal( + 0, + deps.App.WasmKeeper.GetExecuteCount(deps.Ctx, wasmContract), + "first message should not have been executed", +)app/evmante/evmante_can_transfer_test.go (1)
Line range hint
22-40: Consider adding test cases for edge cases in balance conversion.While the test coverage is good, consider adding test cases for:
- Very large token amounts that might test precision limits
- Zero value transfers
- Race conditions in balance checks
x/evm/precompile/test/export.go (1)
328-328: Consider usinguintinstead ofintfor thetimesparameterThe parameter type change from
uinttointcould potentially allow negative values, which doesn't make sense for a counter increment operation.- times int, + times uint,x/evm/keeper/bank_extension_test.go (1)
361-365: Consider extracting transaction parameters for better readability.The transaction setup could be more readable by extracting the parameters into descriptive variables.
- txTransferWei := evmtest.TxTransferWei{ - Deps: &deps, - To: to.EthAddr, - AmountWei: tooManyTokensWei, - } + // Setup transaction with amount exceeding balance to test insufficient funds error + txParams := evmtest.TxTransferWei{ + Deps: &deps, + To: to.EthAddr, + AmountWei: tooManyTokensWei, // Amount > balance to force failure + }x/evm/keeper/call_contract.go (1)
33-39: Update the function documentation to include the newevmObjparameter.The
CallContractWithInputfunction now includes an additional parameterevmObj *vm.EVM. Please update the function comment to reflect this change and describe the purpose and usage of theevmObjparameter.x/evm/keeper/erc20.go (1)
Line range hint
70-282: Update documentation for methods with the newevmObjparameter.Several methods (
Mint,Transfer,Burn,BalanceOf,LoadERC20Name,LoadERC20Symbol,LoadERC20Decimals,loadERC20String,loadERC20Uint8,LoadERC20BigInt) now include anevmObj *vm.EVMparameter. Please update the function documentation to reflect this change and explain the role ofevmObjin these methods.x/evm/keeper/msg_server.go (1)
Line range hint
771-793: Handle tracer setup and execution timeout appropriatelyWhen configuring the tracer and handling timeouts, ensure that the context cancellation and error handling are robust to prevent potential goroutine leaks or incomplete traces.
Consider updating the timeout handling to provide clearer error messages in case of execution timeout.
x/evm/keeper/grpc_query.go (2)
Line range hint
400-415: Handle account nonce and context during gas estimationBy increasing the account nonce and resetting the gas meter, the estimation process becomes more accurate. Verify that this does not introduce inconsistencies in account state.
Ensure that the nonce manipulation does not affect concurrent operations or lead to race conditions.
Line range hint
722-793: Improve error handling and resource management inTraceEthTxMsgEnsure that the tracer is properly configured and that context deadlines are respected to prevent hangs. Enhance error messages for better diagnostics.
Consider logging more detailed information when errors occur during tracing.
x/evm/evmtest/eth_test.go (1)
26-26: Update test function to useNewEthTxMsgAsCmtReplacing individual tests with
NewEthTxMsgAsCmtsimplifies the test suite. Ensure thatNewEthTxMsgAsCmtcovers all necessary cases previously tested.x/evm/keeper/vm_config.go (1)
59-65: Improve error case documentation.The comment "should never happen" suggests an impossible scenario, but the code handles it. Consider:
- Documenting when this case could occur (e.g., during chain initialization or validator set changes)
- Adding logging for this edge case to help with debugging
func (k Keeper) GetCoinbaseAddress(ctx sdk.Context) common.Address { proposerAddress := sdk.ConsAddress(ctx.BlockHeader().ProposerAddress) validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, proposerAddress) if !found { - // should never happen, but just in case, return an empty address - // we don't really care about the coinbase adresss since we're PoS and not PoW + // Return empty address for edge cases: + // - During chain initialization + // - During validator set changes + // - When proposer is not in current validator set + // This is acceptable in PoS systems where coinbase is not as critical as in PoW + k.Logger(ctx).Debug("validator not found for proposer address", "proposer", proposerAddress) return common.Address{} }app/wasmext/wasmext_test.go (2)
27-29: Enhance documentation to explain security implications.Add a comment explaining why the Wasm to EVM call pattern is restricted. This helps future developers understand the security considerations behind this limitation.
-// WasmVM to EVM call pattern is not yet supported. This test verifies the -// Nibiru's [wasmkeeper.Option] function as expected. +// WasmVM to EVM call pattern is intentionally restricted to prevent potential +// security vulnerabilities where malicious Wasm contracts could exploit EVM state +// transitions. This test verifies that Nibiru's [wasmkeeper.Option] function +// correctly enforces this restriction.
76-98: Consider adding more test cases for comprehensive coverage.The test suite would benefit from additional test cases to verify:
- Other message types that should be allowed from Wasm contracts
- Edge cases like empty messages or invalid message types
- Different error scenarios
Would you like me to help generate additional test cases to improve coverage?
x/evm/keeper/funtoken_from_coin_test.go (3)
53-62: Consider adding balance assertions before the insufficient funds test.While the test correctly verifies the insufficient funds error, adding balance assertions before the test would make it more robust and self-documenting.
s.Run("insufficient funds to create funtoken", func() { + // Verify initial balance is 0 + balance := deps.App.BankKeeper.GetBalance(deps.Ctx, deps.Sender.NibiruAddr, bankDenom) + s.Require().True(balance.IsZero(), "expected zero initial balance") + s.T().Log("sad: not enough funds to create fun token") _, err := deps.EvmKeeper.CreateFunToken( sdk.WrapSDKContext(deps.Ctx),
205-209: Consider consolidating balance assertions into a helper function.Multiple balance assertion blocks with similar patterns could be consolidated into a helper function to reduce code duplication.
+func assertBalances(t *testing.T, deps evmtest.TestDeps, evmObj *evm.EVM, erc20Addr gethcommon.Address, accounts []gethcommon.Address, expectedBalances []*big.Int, descriptions []string) { + for i, acc := range accounts { + evmtest.AssertERC20BalanceEqualWithDescription( + t, deps, evmObj, erc20Addr, acc, expectedBalances[i], descriptions[i], + ) + } +}
355-361: Consider using named parameters for better readability.The contract input packing could be more readable with named parameters in a struct.
+type NativeSendThenPrecompileSendParams struct { + to string + amount *big.Int + bankTo string + bankAmount *big.Int +} + contractInput, err := embeds.SmartContract_TestNativeSendThenPrecompileSendJson.ABI.Pack( "nativeSendThenPrecompileSend", - alice.EthAddr, /*to*/ - newSendAmtEvmTransfer, /*amount*/ - alice.NibiruAddr.String(), /*to*/ - newSendAmtSendToBank, /*amount*/ + params.to, + params.amount, + params.bankTo, + params.bankAmount, )x/evm/precompile/funtoken_test.go (2)
83-97: Consider adding timeout context for contract calls.While the test setup is good, adding a timeout context for contract calls would prevent potential test hangs.
+import "context" +import "time" callWhoAmI := func(arg string) (evmResp *evm.MsgEthereumTxResponse, err error) { + ctx, cancel := context.WithTimeout(deps.Ctx, 5*time.Second) + defer cancel() fmt.Printf("arg: %s", arg) contractInput, err := embeds.SmartContract_FunToken.ABI.Pack("whoAmI", arg) require.NoError(t, err) evmObj, _ := deps.NewEVM() return deps.EvmKeeper.CallContractWithInput( - deps.Ctx, + ctx, evmObj,
674-686: Consider adding validation for parsed values.The response parsing could benefit from basic validation of the parsed values.
func (out FunTokenBankBalanceReturn) ParseFromResp( evmResp *evm.MsgEthereumTxResponse, ) (bal *big.Int, ethAddr gethcommon.Address, bech32Addr string, err error) { err = embeds.SmartContract_FunToken.ABI.UnpackIntoInterface( &out, "bankBalance", evmResp.Ret, ) if err != nil { return } + if out.BankBal == nil { + return nil, gethcommon.Address{}, "", fmt.Errorf("nil bank balance") + } + if out.NibiruAcc.Bech32Addr == "" { + return nil, gethcommon.Address{}, "", fmt.Errorf("empty bech32 address") + } return out.BankBal, out.NibiruAcc.EthAddr, out.NibiruAcc.Bech32Addr, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
CHANGELOG.md(1 hunks)app/evmante/evmante_can_transfer.go(3 hunks)app/evmante/evmante_can_transfer_test.go(1 hunks)app/wasmext/wasmext_test.go(1 hunks)eth/rpc/backend/backend_suite_test.go(3 hunks)eth/rpc/backend/gas_used_test.go(0 hunks)x/common/testutil/testnetwork/network.go(1 hunks)x/evm/evmmodule/genesis_test.go(4 hunks)x/evm/evmtest/erc20.go(3 hunks)x/evm/evmtest/eth_test.go(1 hunks)x/evm/evmtest/evmante.go(2 hunks)x/evm/evmtest/test_deps.go(2 hunks)x/evm/evmtest/tx.go(4 hunks)x/evm/evmtest/tx_test.go(1 hunks)x/evm/keeper/bank_extension_test.go(2 hunks)x/evm/keeper/call_contract.go(3 hunks)x/evm/keeper/erc20.go(9 hunks)x/evm/keeper/erc20_test.go(1 hunks)x/evm/keeper/funtoken_from_coin.go(2 hunks)x/evm/keeper/funtoken_from_coin_test.go(18 hunks)x/evm/keeper/funtoken_from_erc20.go(4 hunks)x/evm/keeper/funtoken_from_erc20_test.go(11 hunks)x/evm/keeper/grpc_query.go(16 hunks)x/evm/keeper/msg_server.go(18 hunks)x/evm/keeper/random_test.go(1 hunks)x/evm/keeper/vm_config.go(3 hunks)x/evm/msg.go(0 hunks)x/evm/precompile/funtoken.go(9 hunks)x/evm/precompile/funtoken_test.go(8 hunks)x/evm/precompile/nibiru_evm_utils_test.go(2 hunks)x/evm/precompile/oracle_test.go(2 hunks)x/evm/precompile/test/export.go(8 hunks)x/evm/precompile/wasm_test.go(9 hunks)x/evm/statedb/journal_test.go(2 hunks)x/evm/statedb/statedb.go(2 hunks)x/evm/statedb/statedb_test.go(2 hunks)x/evm/tx.go(1 hunks)
💤 Files with no reviewable changes (2)
- eth/rpc/backend/gas_used_test.go
- x/evm/msg.go
✅ Files skipped from review due to trivial changes (1)
- x/evm/tx.go
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
📓 Learnings (2)
x/evm/keeper/funtoken_from_erc20_test.go (1)
Learnt from: k-yang
PR: NibiruChain/nibiru#2129
File: x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol:24-32
Timestamp: 2024-12-26T04:43:43.966Z
Learning: The `TestInfiniteRecursionERC20` contract is intentionally designed to test infinite recursion scenarios in ERC20-based contracts for the `FunToken` architecture.
x/evm/evmtest/test_deps.go (1)
Learnt from: k-yang
PR: NibiruChain/nibiru#2165
File: x/evm/keeper/funtoken_from_coin_test.go:30-30
Timestamp: 2025-01-17T17:32:33.412Z
Learning: In test dependencies, `deps.NewEVM()` returns both an EVM object and a StateDB object with the signature `(*vm.EVM, *statedb.StateDB)`. The method is used in test files to create new EVM and StateDB instances for testing purposes.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (81)
eth/rpc/backend/backend_suite_test.go (2)
105-105: LGTM! Clear parameter documentation.The added comment clearly documents the purpose of the
waitForNextBlockparameter.
154-154: LGTM! Improved numeric readability.The gas value reformatting from
1500_000to1_500_000improves readability while maintaining the same value.x/evm/precompile/nibiru_evm_utils_test.go (3)
6-6: LGTM!The addition of the "testing" import is appropriate for the new test suite runner.
28-30: LGTM! Well-structured test suite setup.The test suite runner follows Go testing conventions and properly integrates with the testify framework.
28-30: Consider adding tests for Wasm-EVM interaction restrictions.Given that this PR aims to prevent Wasm contracts from sending MsgEthereumTx, consider adding test cases that verify this restriction. This would help ensure the security boundary between Wasm and EVM is properly enforced.
Would you like me to help draft test cases that verify Wasm contracts cannot send MsgEthereumTx?
x/evm/evmtest/evmante.go (1)
Line range hint
1-89: Consider adding test cases for Wasm contract restrictions.Given that the PR's main objective is to prevent Wasm contracts from sending MsgEthereumTx, it would be beneficial to add specific test cases that verify this restriction.
Would you like me to help create test cases that verify Wasm contracts cannot send MsgEthereumTx? This would help ensure the PR's main objective is properly tested.
x/evm/precompile/wasm_test.go (3)
33-35: LGTM! Standard test suite setup.The test suite setup follows Go testing best practices.
120-138: LGTM! Well-structured test with proper state verification.The test effectively verifies the counter contract's behavior through multiple executions with appropriate state checks between operations.
Line range hint
360-492: LGTM! Comprehensive validation test coverage.The test provides excellent coverage of multi-message validation scenarios, including proper setup and both positive and negative cases.
x/common/testutil/testnetwork/network.go (1)
497-497: LGTM! Verify impact on CI pipeline duration.Increasing the timeout from 40s to 5min is reasonable for improving test stability, especially with the new Wasm message handler validation. However, this change could affect CI pipeline duration.
Let's check the historical CI job durations to assess the impact:
app/evmante/evmante_can_transfer.go (2)
Line range hint
41-60: LGTM: Gas fee validation logic is correct.The implementation properly validates that the gas fee cap is not less than the block's base fee, with clear error messaging.
67-80: Verify the balance conversion logic.The code now uses the Bank module for balance checks instead of statedb. While this simplifies the implementation, we should ensure that:
- The conversion between native tokens and wei maintains precision
- The balance check is atomic with the actual transfer
Let's verify the usage of these conversion functions:
✅ Verification successful
Balance conversion and check implementation is correct and safe.
The implementation:
- Maintains precision through simple multiplication (1 unibi = 10^12 wei)
- Performs balance check atomically with transfer due to ante handler guarantees
- Uses consistent conversion logic across the codebase
- Has comprehensive test coverage validating precision and correctness
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of NativeToWei to ensure consistent conversion rg "NativeToWei" -A 2 -B 2 # Check for potential race conditions in balance checks ast-grep --pattern 'GetBalance($_)'Length of output: 18761
app/evmante/evmante_can_transfer_test.go (1)
18-18: LGTM: Removed unused field.Good cleanup removing the unused
ctxSetupfield while maintaining comprehensive test coverage.x/evm/precompile/test/export.go (5)
33-34: LGTM! Enhanced contract instantiation with EVM integrationThe changes improve the contract instantiation process by:
- Adding EVM object parameter for better integration
- Using
CallContractWithInputfor more precise control over contract callsAlso applies to: 65-77
176-189: LGTM! Improved query execution with proper EVM integrationThe changes correctly integrate EVM object for contract queries and properly handle the response unpacking.
267-271: LGTM! Enhanced multi-execution support with clearer parameter namingThe changes improve the function by:
- Adding EVM integration
- Renaming
finalizeTxtocommitfor better clarity- Properly handling transaction commitment
Also applies to: 311-316
350-356: LGTM! Improved message construction with cleaner loop logicThe refactored loop-based construction of
executeMsgsis more efficient and readable.
367-369: Verify gas limit constant usageThe use of
serverconfig.DefaultEthCallGasLimitinstead of the customWasmGasLimitExecuteconstant differs from other similar calls in this file.x/evm/keeper/bank_extension_test.go (2)
366-366: LGTM!The error handling is appropriate for testing the insufficient balance scenario.
425-425: 🛠️ Refactor suggestionConsider using state comparison instead of pointer comparison.
The current pointer comparison (
first == db.StateDB) might be fragile as it assumes pointer reuse. A more robust approach would be to compare the actual state data.- s.True(first == db.StateDB, db.Explanation) + // Compare state data instead of pointers for more reliable verification + s.True(first.Equal(db.StateDB), fmt.Sprintf("StateDB should be unchanged: %s", db.Explanation))This change would make the test more reliable by:
- Verifying actual state equality rather than pointer identity
- Detecting cases where state was modified and restored
- Making the test less dependent on implementation details
Let's verify if the
Equalmethod exists:x/evm/keeper/msg_server.go (16)
50-50: Simplify EVM configuration retrievalReplacing the previous method parameters with
k.GetEVMConfig(ctx)improves code readability and maintainability by directly using the context.
53-55: Ensure correct signer instantiationUpdating the signer to use
gethcore.NewLondonSigner(evmCfg.ChainConfig.ChainID)is appropriate for the London hard fork. Verify that this change is compatible with all intended Ethereum chain configurations.
59-64: HandlestateDBinitialization carefullyThe introduction of
stateDBinitialization ensures that it is notnilbefore proceeding. However, ensure thatk.Bank.StateDBis appropriately managed elsewhere to prevent unintended side effects.
77-77: Calculate gas price accuratelyUsing
txMsg.EffectiveGasPriceWeiPerGas(evmCfg.BaseFeeWei)to calculateweiPerGasis appropriate. Confirm thatEffectiveGasPriceWeiPerGascorrectly accounts for base fees and tips under different EVM configurations.
Line range hint
109-135: Ensure proper EVM initialization inNewEVMThe changes in the
NewEVMfunction update the block and transaction contexts with the newevmCfg. Ensure that thepseudoRandomvalue is correctly computed and that thevmConfigproperly reflects the updated EVM configurations.
Line range hint
273-284: Check intrinsic gas calculationThe error handling for intrinsic gas overflow has been updated. Confirm that the intrinsic gas is calculated correctly, especially for transactions with large data payloads.
295-295: Prepare the access list appropriatelyMoving the access list preparation into
ApplyEvmMsgensures it's available during calls that don't go through the Ante Handler. Verify that this change doesn't introduce any unintended side effects in transaction processing.
310-312: Manage nonce correctly for state transitionsSetting the sender's nonce before EVM execution and incrementing it afterward ensures proper nonce management. Confirm that this approach maintains consistency, especially in concurrent transaction processing scenarios.
331-331: Increment nonce after processing the messageEnsuring the sender's nonce is incremented after message processing prevents nonce reuse. This change enhances transaction integrity.
341-345: CommitstateDBconditionally and manage cleanupCommitting the
stateDBwhencommitistrueand settingk.Bank.StateDBtonilfor garbage collection is good practice. Ensure that this does not affect other parts of the application relying onk.Bank.StateDB.
368-374: Validate gas refund calculationsThe gas refund logic considers the
fullRefundLeftoverGasflag. Verify that the refund calculations align with EIP-3529 and that the refund does not exceed the allowed limit.
389-391: Return consistent EVM response fieldsEnsure that the
Logs,Hash, and other fields inMsgEthereumTxResponseare correctly populated, especially after the updates to thestateDBand EVM execution flow.
571-576: EnsurestateDBis committed and cleaned upCommitting the
stateDBand settingk.Bank.StateDBtonilis important for consistency and resource management. Verify that these changes do not interfere with other operations that may rely onstateDB.
598-603: InitializestateDBbefore ERC20 transferProperly initializing
stateDBensures that the subsequent ERC20 transfer operations have access to the correct state. Confirm that this initialization aligns with the application's state management practices.
662-667: CommitstateDBand clean up after transferAfter the ERC20 transfer, committing the
stateDBand resettingk.Bank.StateDBis essential. Ensure that this process does not leave the application in an inconsistent state.
244-252: 🛠️ Refactor suggestionUpdate
ApplyEvmMsgmethod signatureAdding the
evmObjparameter toApplyEvmMsgmakes the EVM instance explicit, improving clarity. Ensure that all calls to this method are updated accordingly throughout the codebase.x/evm/keeper/grpc_query.go (13)
273-273: Simplify EVM configuration retrieval inEthCallUsing
evmCfg := k.GetEVMConfig(ctx)streamlines the configuration retrieval process. This change improves maintainability.
279-289: Ensure correct message creation and state initializationUpdating the nonce and creating the message with
args.ToMessageensures accuracy. Properly initializingstateDBwithstatedb.Newsets up the execution environment correctly.
327-327: Retrieve EVM configuration inEstimateGasForEvmCallTypeConsistently using
evmCfg := k.GetEVMConfig(ctx)aligns with changes made in other functions for configuration retrieval.
372-372: Create message with updated EVM configurationEnsure that the message creation with
args.ToMessageuses the correct base fee fromevmCfg.BaseFeeWeifor accurate gas estimation.
382-393: Update message gas dynamically during estimationModifying the message gas within the
executablefunction allows for accurate binary search in gas estimation. Confirm that this approach correctly updates all necessary message parameters.
422-424: InitializestateDBandevmproperly for estimationCreating a new
stateDBandevmObjwithin the estimation context ensures isolation and accuracy in gas estimation.
491-498: Compute and set base fee inTraceTxBy recalculating the
BaseFeeWeibased on the context, the tracing reflects the correct execution environment. Confirm that this calculation is accurate for different block heights.
Line range hint
507-517: Process predecessors correctly inTraceTxEnsure that prior transactions are applied correctly to maintain state consistency before tracing the target transaction. Verify that the state reflects all predecessors accurately.
538-543: Handle message creation and tracing inTraceTxCreating the message with the correct signer and base fee is crucial for accurate tracing. Ensure that any errors are properly handled and that the tracing results are reliable.
586-591: Set base fee correctly inTraceCallConsistently computing and setting
evmCfg.BaseFeeWeiensures that theTraceCallfunction uses the correct base fee for tracing.
Line range hint
608-621: CreateevmMsgcorrectly for unsigned transactionsSince
req.Msgis not signed, constructingevmMsgdirectly ensures that the tracing operates on the intended transaction parameters.
669-674: Compute base fee for block tracingIn
TraceBlock, computing the base fee using the context ensures that all transactions within the block are traced with the correct fee parameters.
Line range hint
682-698: Process transactions correctly inTraceBlockIterating over each transaction and handling errors individually allows for comprehensive tracing results, even if some transactions fail.
x/evm/evmtest/tx_test.go (1)
34-35: LGTM! Enhanced test clarity.Good improvement to add a descriptive message to the balance assertion, making test failures more informative.
x/evm/keeper/random_test.go (2)
14-14: LGTM! Proper EVM context initialization.Good practice to create and use a dedicated EVM object for contract interactions.
Also applies to: 20-21
29-31: LGTM! Consistent EVM context management.Correctly recreating the EVM object before the second random value check ensures a clean state for each test case.
x/evm/evmtest/test_deps.go (2)
42-42: LGTM! Simplified initialization.Good refactoring to directly use NewEthPrivAcc() for Sender initialization.
55-59: LGTM! Well-structured EVM initialization.The NewEVM method correctly creates and returns both EVM and StateDB objects, following the expected pattern.
x/evm/keeper/vm_config.go (1)
14-20: LGTM! Streamlined EVM configuration.Good simplification of the GetEVMConfig method by removing unnecessary error handling and directly returning the config.
x/evm/keeper/erc20_test.go (1)
14-102: Well-structured test improvements!The changes significantly improve test readability and maintainability by:
- Using descriptive test case names
- Isolating each test case in its own s.Run block
- Including helpful error messages in assertions
- Following a consistent pattern for setup and verification
x/evm/evmmodule/genesis_test.go (1)
Line range hint
51-127: LGTM! Consistent updates to include evmObj parameter.The changes correctly update all ERC20 method calls to include the evmObj parameter while maintaining the original test logic and coverage.
x/evm/precompile/oracle_test.go (1)
65-78: LGTM! Improved contract call pattern.The changes enhance the test by using the ABI to pack the input data before calling the contract, which is a more robust approach. The error handling for the packing operation is properly implemented.
Also applies to: 129-143
x/evm/keeper/funtoken_from_erc20.go (2)
31-32: LGTM! Consistent parameter addition.The addition of the
evmObjparameter and its usage in ERC20 calls aligns with the codebase's pattern for EVM interactions.Also applies to: 36-46
122-141: LGTM! Proper state management.The changes implement proper state database initialization, EVM message construction, and cleanup. The error handling is comprehensive, and the state is correctly committed.
Also applies to: 175-181
x/evm/statedb/journal_test.go (5)
24-51: LGTM! Comprehensive test for state cleanup.The test effectively verifies that the state database is properly cleaned up after committing changes, with appropriate setup and verification steps.
53-78: LGTM! Well-structured state database test.The test effectively validates the state database's behavior with balance operations, including proper verification steps and helpful debug information.
Line range hint
80-160: LGTM! Comprehensive contract interaction test.The test effectively covers both successful and failed contract interactions, with proper state verification and error handling.
162-239: LGTM! Thorough journal reversion test.The test effectively validates snapshot and revert operations, with proper error handling and state verification at each step.
242-247: LGTM! Improved debugging helper.The function provides comprehensive debugging information with proper nil checks.
x/evm/evmtest/tx.go (2)
69-69: LGTM! Consistent nonce retrieval.The changes improve consistency by using the keeper's method for nonce retrieval instead of directly accessing the state database.
Also applies to: 206-206
362-374: LGTM! Useful mock message for testing.The added mock message is well-constructed and provides a reusable test fixture.
x/evm/statedb/statedb_test.go (1)
11-11: LGTM! Clean test suite refactoring.The changes improve the test suite organization by:
- Using direct import instead of aliased import for better readability
- Updating the test suite structure consistently
Also applies to: 33-33, 37-37
x/evm/keeper/funtoken_from_erc20_test.go (2)
58-67: Well-structured test organization using sub-tests.The use of
s.Run()for test cases improves:
- Test isolation
- Error reporting clarity
- Test output readability
202-211: Consistent error handling in EVM operations.The EVM object handling is consistent with the contract execution pattern:
- Preparing contract input
- Creating new EVM instance
- Executing contract with proper gas limits
x/evm/statedb/statedb.go (2)
264-268: Improved context handling for state access.The context selection logic ensures proper state isolation:
- Uses cache context when available
- Falls back to commit context otherwise
329-333: Consistent context handling in storage iteration.The same context selection pattern is applied to storage iteration, maintaining consistency with state access.
x/evm/precompile/funtoken.go (3)
174-182: Enhanced token transfer security with proper EVM context.The EVM module account transfer now properly uses the EVM context, ensuring:
- Accurate gas accounting
- Proper state transitions
- Consistent error handling
594-600: Secure token minting with proper context propagation.The
mintOrUnescrowERC20operation now receives the EVM context, ensuring:
- Proper state management
- Accurate gas accounting
- Consistent error handling across the token lifecycle
644-650: Improved token transfer with balance verification.The transfer operation now returns the actual balance increase, providing:
- Better verification of transferred amounts
- Enhanced error detection
- Improved transaction integrity
x/evm/keeper/funtoken_from_coin_test.go (2)
26-35: LGTM! Well-structured test setup.The test case effectively verifies the contract address computation and ERC20 metadata lookup failure scenario.
82-138: LGTM! Comprehensive happy path test with proper assertions.The test case thoroughly verifies:
- Contract deployment
- Balance checks
- Event emission
- ERC20 metadata verification
x/evm/precompile/funtoken_test.go (2)
Line range hint
32-77: LGTM! Comprehensive error case testing for ABI packing.The test cases effectively cover various error scenarios for ABI packing with clear error messages and assertions.
601-623: LGTM! Well-structured response parsing types.The
FunTokenWhoAmIReturnstruct and its parsing method are well-designed with proper error handling.x/evm/keeper/funtoken_from_coin.go (2)
82-94: Verify the gas price parameters in the EVM message.In the construction of
evmMsg, the gas-related parametersgasFeeCap,gasTipCap, andgasPriceare all set to zero. Ensure that this is intentional and won't cause issues with EVM execution or gas calculations.Run the following script to compare how
gethcore.NewMessageis used elsewhere in the codebase and inspect the gas parameters:✅ Verification successful
Zero gas price parameters are intentional for internal module operations
The zero values for
gasFeeCap,gasTipCap, andgasPriceare consistent with all other internal module operations in the codebase, where the sender isEVM_MODULE_ADDRESS. Only external transaction handling uses actual gas values, while internal operations correctly use zero gas prices with appropriate gas limits.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of `gethcore.NewMessage` and inspect gas parameters. # Search for `gethcore.NewMessage` usages in the codebase rg 'gethcore\.NewMessage\(' -A 10Length of output: 4480
109-114: Review the lifecycle management ofstateDB.After committing
stateDB, it is set tonilwithk.Bank.StateDB = nil. Verify that no other parts of the application rely onk.Bank.StateDBafter this operation. Improper handling may lead to nil pointer dereferences or state inconsistencies.Run the following script to identify other usages of
k.Bank.StateDBin the codebase:✅ Verification successful
StateDB lifecycle management is properly implemented
The current implementation follows a consistent and safe pattern across all files. The StateDB is properly initialized when needed, committed when operations are complete, and explicitly set to nil to prevent usage after commit. No instances of unsafe post-commit usage were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all references to `k.Bank.StateDB` in the codebase. # Search for usages of `k.Bank.StateDB` rg 'k\.Bank\.StateDB'Length of output: 613
Script:
#!/bin/bash # Show context around each usage of StateDB to verify the pattern rg -B 5 -A 5 'k\.Bank\.StateDB' # Find any potential uses after nil assignment using ast-grep ast-grep --pattern 'k.Bank.StateDB = nil $$$ $_= k.Bank.StateDB'Length of output: 7345
| contractInput, err := embeds.SmartContract_ERC20Minter.ABI.Pack("transfer", recipient, coin.Amount.BigInt()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| evmMsg := gethcore.NewMessage( |
There was a problem hiding this comment.
Handle error when packing transfer method call
Similar to the minting process, ensure that any errors during the packing of the transfer method are properly handled and informative.
Apply this diff to enhance error handling:
contractInput, err := embeds.SmartContract_ERC20Minter.ABI.Pack("transfer", recipient, coin.Amount.BigInt())
if err != nil {
- return nil, err
+ return nil, errors.Wrap(err, "failed to pack 'transfer' method call for ERC20 contract")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contractInput, err := embeds.SmartContract_ERC20Minter.ABI.Pack("transfer", recipient, coin.Amount.BigInt()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| evmMsg := gethcore.NewMessage( | |
| contractInput, err := embeds.SmartContract_ERC20Minter.ABI.Pack("transfer", recipient, coin.Amount.BigInt()) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "failed to pack 'transfer' method call for ERC20 contract") | |
| } | |
| evmMsg := gethcore.NewMessage( |
| contractInput, err := embeds.SmartContract_ERC20Minter.ABI.Pack("mint", recipient, coin.Amount.BigInt()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| evmMsg := gethcore.NewMessage( | ||
| evm.EVM_MODULE_ADDRESS, | ||
| &erc20Addr, | ||
| k.GetAccNonce(ctx, evm.EVM_MODULE_ADDRESS), | ||
| big.NewInt(0), | ||
| Erc20GasLimitExecute, | ||
| big.NewInt(0), | ||
| big.NewInt(0), | ||
| big.NewInt(0), | ||
| contractInput, | ||
| gethcore.AccessList{}, | ||
| true, | ||
| ) |
There was a problem hiding this comment.
Handle potential errors in ERC20 minting
When packing the mint method call for the ERC20 contract, ensure error handling is comprehensive. If embeds.SmartContract_ERC20Minter.ABI.Pack fails, it could indicate an issue with the contract ABI or parameters.
Apply this diff to enhance error handling:
func (k Keeper) convertCoinToEvmBornCoin(
ctx sdk.Context,
sender sdk.AccAddress,
recipient gethcommon.Address,
coin sdk.Coin,
funTokenMapping evm.FunToken,
) (*evm.MsgConvertCoinToEvmResponse, error) {
// ... existing code ...
contractInput, err := embeds.SmartContract_ERC20Minter.ABI.Pack("mint", recipient, coin.Amount.BigInt())
if err != nil {
- return nil, err
+ return nil, errors.Wrap(err, "failed to pack 'mint' method call for ERC20 contract")
}
// ... existing code ...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contractInput, err := embeds.SmartContract_ERC20Minter.ABI.Pack("mint", recipient, coin.Amount.BigInt()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| evmMsg := gethcore.NewMessage( | |
| evm.EVM_MODULE_ADDRESS, | |
| &erc20Addr, | |
| k.GetAccNonce(ctx, evm.EVM_MODULE_ADDRESS), | |
| big.NewInt(0), | |
| Erc20GasLimitExecute, | |
| big.NewInt(0), | |
| big.NewInt(0), | |
| big.NewInt(0), | |
| contractInput, | |
| gethcore.AccessList{}, | |
| true, | |
| ) | |
| contractInput, err := embeds.SmartContract_ERC20Minter.ABI.Pack("mint", recipient, coin.Amount.BigInt()) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "failed to pack 'mint' method call for ERC20 contract") | |
| } | |
| evmMsg := gethcore.NewMessage( | |
| evm.EVM_MODULE_ADDRESS, | |
| &erc20Addr, | |
| k.GetAccNonce(ctx, evm.EVM_MODULE_ADDRESS), | |
| big.NewInt(0), | |
| Erc20GasLimitExecute, | |
| big.NewInt(0), | |
| big.NewInt(0), | |
| big.NewInt(0), | |
| contractInput, | |
| gethcore.AccessList{}, | |
| true, | |
| ) |
| var errSuffix string | ||
| if description == "" { | ||
| errSuffix = description | ||
| } else { | ||
| errSuffix = ": " + description | ||
| } |
There was a problem hiding this comment.
Fix duplicate error suffix logic.
The same incorrect logic exists in AssertBankBalanceEqualWithDescription.
var errSuffix string
if description == "" {
- errSuffix = description
-} else {
- errSuffix = ": " + description
+ errSuffix = ""
+} else {
+ errSuffix = ": " + description
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var errSuffix string | |
| if description == "" { | |
| errSuffix = description | |
| } else { | |
| errSuffix = ": " + description | |
| } | |
| var errSuffix string | |
| if description == "" { | |
| errSuffix = "" | |
| } else { | |
| errSuffix = ": " + description | |
| } |
| var errSuffix string | ||
| if description == "" { | ||
| errSuffix = description | ||
| } else { | ||
| errSuffix = ": " + description | ||
| } |
There was a problem hiding this comment.
Fix incorrect error suffix logic.
The current logic assigns an empty description to errSuffix when description is empty, which is the opposite of what's intended.
var errSuffix string
if description == "" {
- errSuffix = description
-} else {
- errSuffix = ": " + description
+ errSuffix = ""
+} else {
+ errSuffix = ": " + description
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var errSuffix string | |
| if description == "" { | |
| errSuffix = description | |
| } else { | |
| errSuffix = ": " + description | |
| } | |
| var errSuffix string | |
| if description == "" { | |
| errSuffix = "" | |
| } else { | |
| errSuffix = ": " + description | |
| } |
| stateDB := k.Bank.StateDB | ||
| if stateDB == nil { | ||
| stateDB = k.NewStateDB(ctx, txConfig) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct access to k.Bank.StateDB.
Directly accessing k.Bank.StateDB may lead to concurrency issues or unintended side effects. It's recommended to manage stateDB through dedicated methods or within the EVM context to ensure proper encapsulation and thread safety.
Purpose / Abstract
nibid q evm accountis not symmetric for wasm addresses #2138Note that almost all of this
DispatchMsglogic is copied directly out of wasmd due to its use of private structs and functions.Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Release Notes
New Features
Bug Fixes
Chores
Performance
These release notes provide a high-level overview of the significant changes across the Nibiru EVM module, focusing on improvements in functionality, error handling, and code organization.