feat(evm-precompile): Precompile for one-way EVM calls to invoke/execute Wasm contracts. #2054
Conversation
WalkthroughThe changes encompass extensive updates to the Nibiru blockchain, enhancing its functionality across various modules, including the inflation, perpetual, and EVM integration. Key modifications include the introduction of new methods, updates to existing method signatures, and the addition of comprehensive error handling and precompile capabilities for Wasm contracts. Documentation has been updated to reflect these changes, ensuring clarity and consistency in the project structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EVM
participant Wasm
User->>EVM: Execute Wasm Contract
EVM->>Wasm: Call execute method
Wasm-->>EVM: Return execution result
EVM-->>User: Return result
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 #2054 +/- ##
==========================================
- Coverage 65.57% 65.45% -0.12%
==========================================
Files 265 268 +3
Lines 16689 16985 +296
==========================================
+ Hits 10943 11117 +174
- Misses 4858 4941 +83
- Partials 888 927 +39
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Outside diff range and nitpick comments (17)
x/evm/embeds/artifacts/contracts/FunToken.sol/IFunToken.json (1)
Line range hint
6-26: Consider improving thebankSendfunction design.While the ABI remains unchanged, there are potential improvements to consider for the
bankSendfunction:
- The function name "bankSend" might not be the most descriptive. Consider renaming it to something like "transferERC20ToBech32Address" to better reflect its purpose.
- Using a string for the recipient address is unusual in EVM contracts. This might be intended for cross-chain compatibility (e.g., with Cosmos addresses), but it could lead to confusion for EVM developers. Consider adding a comment explaining this design choice.
- The function doesn't return any value or emit an event. This could make it difficult to track or verify transactions. Consider adding an event emission or a return value to confirm the success of the transfer.
Here's a suggested improvement to the ABI:
"abi": [ { "inputs": [ { "internalType": "address", "name": "erc20", "type": "address" }, { "internalType": "uint256", "name": "amount", "type": "uint256" }, { "internalType": "string", "name": "toBech32Address", "type": "string" } ], "name": "transferERC20ToBech32Address", "outputs": [ { "internalType": "bool", "name": "success", "type": "bool" } ], "stateMutability": "nonpayable", "type": "function" }, { "anonymous": false, "inputs": [ { "indexed": true, "internalType": "address", "name": "erc20", "type": "address" }, { "indexed": false, "internalType": "uint256", "name": "amount", "type": "uint256" }, { "indexed": true, "internalType": "string", "name": "toBech32Address", "type": "string" } ], "name": "ERC20TransferredToBech32Address", "type": "event" } ]This suggestion improves clarity, adds a return value, and includes an event for better traceability.
x/evm/embeds/README.md (1)
3-8: LGTM! Consider adding Node.js version information.The "Hacking" section provides clear and concise instructions for setting up and compiling the project. This is a valuable addition to the documentation.
Consider adding information about the required or recommended Node.js version to ensure developers have the correct environment set up.
x/evm/embeds/embeds.go (3)
37-42: LGTM: SmartContract_FunToken updated for precompile functionalityThe changes to
SmartContract_FunTokenare consistent with the PR objectives. The use offuntokenPrecompileJSONand the updated comment reflect its new role as a precompile contract interface.Consider updating the comment to be more specific:
- // SmartContract_Funtoken: Precompile contract interface for + // SmartContract_FunToken: Precompile contract interface forThis change would correct the capitalization of "FunToken" in the comment.
44-49: LGTM: SmartContract_Wasm added for Wasm precompile functionalityThe addition of
SmartContract_Wasmdirectly addresses the PR objective of implementing a precompile for Wasm contract execution. The structure and naming convention are consistent with other smart contract declarations in the file.Consider updating the comment to be more specific:
- // SmartContract_Funtoken: Precompile contract interface for + // SmartContract_Wasm: Precompile contract interface forThis change would correct the contract name in the comment.
Line range hint
1-101: Summary: Wasm precompile implementation successfully integratedThe changes in this file successfully implement the Wasm precompile functionality as outlined in the PR objectives. Key points:
- New embedded JSON files for FunToken and Wasm precompiles have been added.
- The
SmartContract_FunTokenhas been updated to use the new precompile JSON.- A new
SmartContract_Wasmhas been introduced for Wasm contract execution.- The
initfunction has been updated to load the new Wasm precompile.These changes provide the necessary groundwork for enabling one-way EVM calls to invoke/execute Wasm contracts, aligning well with the PR's goals.
As the project evolves, consider creating a separate package for precompile-related code if the number of precompiles continues to grow. This would improve modularity and make it easier to manage and test precompile implementations independently.
x/evm/precompile/funtoken_test.go (1)
20-24: LGTM! Consider adding a brief comment for WasmSuite.The updated TestSuite function now runs both FuntokenSuite and WasmSuite, which aligns well with the PR objective of implementing a precompile for Wasm contract execution. This separation of concerns improves the overall organization of the tests.
Consider adding a brief comment for the WasmSuite, similar to the one for TestSuite, to maintain consistency and improve code documentation:
// TestSuite: Runs all the tests in the suite. func TestSuite(t *testing.T) { suite.Run(t, new(FuntokenSuite)) + // WasmSuite: Tests for Wasm contract execution precompile suite.Run(t, new(WasmSuite)) }x/common/testutil/testnetwork/start_node.go (1)
145-148: LGTM! Consider enhancing error handling.The simplification of the JSON-RPC server startup process is a good improvement. It removes unnecessary code blocks while maintaining the essential functionality.
Consider wrapping the error with additional context about the JSON-RPC server:
val.jsonrpc, val.jsonrpcDone, err = server.StartJSONRPC(val.Ctx, val.ClientCtx, tmRPCAddr, tmEndpoint, val.AppConfig, val.EthTxIndexer) if err != nil { - return errors.Wrap(err, "failed to start JSON-RPC server") + return errors.Wrap(err, fmt.Sprintf("failed to start JSON-RPC server at %s", val.AppConfig.JSONRPC.Address)) }This change would provide more specific information about where the JSON-RPC server failed to start, which could be helpful for debugging.
README.md (2)
31-31: LGTM! Consider adding a brief explanation for the repository name change.The update to the Wasm module description and link is accurate and provides more detailed information about the CosmWasm smart contracts. This change aligns well with the project's evolution.
Consider adding a brief note explaining the reason for changing the repository name from "cw-nibiru" to "nibiru-wasm". This could help long-time contributors understand the transition. For example:
- | [Wasm][code-x-wasm] | Implements the execution environment for WebAssembly (WASM) smart contracts. CosmWasm smart contracts are Rust-based, Wasm smart contracts built for enhanced security, performance, and interoperability. See our [CosmWasm sandbox monorepo (nibiru-wasm)](https://github.com/NibiruChain/nibiru-wasm/tree/main) for the protocol's core smart contracts. | + | [Wasm][code-x-wasm] | Implements the execution environment for WebAssembly (WASM) smart contracts. CosmWasm smart contracts are Rust-based, Wasm smart contracts built for enhanced security, performance, and interoperability. See our [CosmWasm sandbox monorepo (nibiru-wasm)](https://github.com/NibiruChain/nibiru-wasm/tree/main) (formerly known as cw-nibiru) for the protocol's core smart contracts. This repository has been renamed to better reflect its role in the Nibiru ecosystem. |
32-32: Consider updating the EVM module description to include the new precompile feature.The README provides a comprehensive overview of the Nibiru Chain project. However, given that this PR implements a precompile for one-way EVM calls to invoke/execute Wasm contracts (as mentioned in the PR objectives), it would be beneficial to update the EVM module description to reflect this new feature.
Consider updating the EVM module description in the Components of Nibiru table. Here's a suggested change:
- | [EVM][code-x-evm] | Implements Nibiru EVM, which manages an Ethereum Virtual Machine (EVM) state database and enables the execution of Ethereum smart contracts. Nibiru EVM is an extension of "[geth](https://github.com/ethereum/go-ethereum)" along with "web3" and "eth" JSON-RPC methods. | + | [EVM][code-x-evm] | Implements Nibiru EVM, which manages an Ethereum Virtual Machine (EVM) state database and enables the execution of Ethereum smart contracts. Nibiru EVM is an extension of "[geth](https://github.com/ethereum/go-ethereum)" along with "web3" and "eth" JSON-RPC methods. It also includes a precompile feature for one-way EVM calls to invoke/execute Wasm contracts, enhancing interoperability between EVM and Wasm environments. |This addition would highlight the new feature and its significance in improving interoperability within the Nibiru ecosystem.
x/evm/embeds/contracts/Wasm.sol (1)
18-20: Clarify the usage offundsparameter inexecutefunctionThe documentation mentions that supplying funds is uncommon and an empty array is usually passed. Consider adding default behavior or overloading the function to handle cases without funds more intuitively.
x/evm/precompile/funtoken.go (1)
208-210: Fix typographical errors in commentsThere are minor typos in the comments that should be corrected for clarity:
- Line 208: "valiated" should be "validated"
- Line 210: "the the" should be "the"
Please update these to improve readability.
x/evm/precompile/wasm.go (3)
93-95: Increase Test Coverage for Error Handling When Loadingsdk.ContextThe error handling when the type assertion
evm.StateDB.(*statedb.StateDB)fails is not covered by tests. Consider adding unit tests to ensure this error path is properly tested.Would you like assistance in writing tests to cover this scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 94-95: x/evm/precompile/wasm.go#L94-L95
Added lines #L94 - L95 were not covered by tests
110-114: Increase Test Coverage for the Default Case in Method DispatchThe default case in the method dispatch is expected to be unreachable, but it's important to have test coverage to ensure that unexpected method names are properly handled. Consider adding a test case to cover this scenario.
Would you like assistance in writing a test case to cover this default path?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 110-110: x/evm/precompile/wasm.go#L110
Added line #L110 was not covered by tests
[warning] 113-114: x/evm/precompile/wasm.go#L113-L114
Added lines #L113 - L114 were not covered by tests
358-360: Fix Typographical Errors in CommentsPlease correct the typos in the comments:
- "valiated" should be "validated".
- "the the" should be "the".
x/evm/precompile/wasm_test.go (3)
335-384: Simplify 'incrementWasmCounterWithExecuteMulti' function logicThe loop starting at line 364 can be simplified by using Go's built-in functions to create a slice with duplicated elements. This improves readability and reduces complexity.
Apply this diff to simplify the loop:
func (s *WasmSuite) incrementWasmCounterWithExecuteMulti( deps *evmtest.TestDeps, wasmContract sdk.AccAddress, times uint, ) { // ... existing code ... if times == 0 { executeMsgs = executeMsgs[:0] // force empty } else { - for i := uint(1); i < times; i++ { - executeMsgs = append(executeMsgs, executeMsgs[0]) - } + executeMsgs = append(executeMsgs, make([]struct { + ContractAddr string `json:"contractAddr"` + MsgArgs []byte `json:"msgArgs"` + Funds []precompile.WasmBankCoin `json:"funds"` + }, times-1)...) }
498-587: Handle test dependencies outside the loop in 'TestSadArgsExecute'In the
TestSadArgsExecutefunction, you recreatedepsinside the loop for each test case. Consider movingdeps := evmtest.NewTestDeps()outside the loop to initialize it once, unless each test case requires a fresh instance.
570-585: Add context to error messages in tests for better clarityWhen testing for expected errors, consider adding more context to the
ErrorContainsassertions to make it clear which test case failed and why. This aids in debugging if a test fails.Modify the assertion as follows:
s.Run(tc.name, func() { // ... existing code ... s.ErrorContains(err, tc.wantError, "Test case '%s' failed", tc.name) s.Require().Nil(ethTxResp) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/evm/precompile/hello_world_counter.wasmis excluded by!**/*.wasm
📒 Files selected for processing (20)
- CHANGELOG.md (1 hunks)
- README.md (1 hunks)
- eth/rpc/backend/backend_suite_test.go (2 hunks)
- go.mod (1 hunks)
- x/common/testutil/testnetwork/start_node.go (1 hunks)
- x/evm/embeds/README.md (1 hunks)
- x/evm/embeds/artifacts/contracts/FunToken.sol/IFunToken.json (1 hunks)
- x/evm/embeds/artifacts/contracts/Wasm.sol/IWasm.json (1 hunks)
- x/evm/embeds/contracts/FunToken.sol (1 hunks)
- x/evm/embeds/contracts/Wasm.sol (1 hunks)
- x/evm/embeds/embeds.go (3 hunks)
- x/evm/evmtest/erc20.go (2 hunks)
- x/evm/precompile/errors.go (1 hunks)
- x/evm/precompile/funtoken.go (4 hunks)
- x/evm/precompile/funtoken_test.go (2 hunks)
- x/evm/precompile/precompile.go (3 hunks)
- x/evm/precompile/wasm.go (1 hunks)
- x/evm/precompile/wasm_parse.go (1 hunks)
- x/evm/precompile/wasm_test.go (1 hunks)
- x/tokenfactory/fixture/fixture.go (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- eth/rpc/backend/backend_suite_test.go
- x/evm/embeds/contracts/FunToken.sol
- x/tokenfactory/fixture/fixture.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/evm/evmtest/erc20.go
[warning] 97-100: x/evm/evmtest/erc20.go#L97-L100
Added lines #L97 - L100 were not covered by testsx/evm/precompile/errors.go
[warning] 28-28: x/evm/precompile/errors.go#L28
Added line #L28 was not covered by tests
[warning] 39-41: x/evm/precompile/errors.go#L39-L41
Added lines #L39 - L41 were not covered by testsx/evm/precompile/funtoken.go
[warning] 126-127: x/evm/precompile/funtoken.go#L126-L127
Added lines #L126 - L127 were not covered by testsx/evm/precompile/precompile.go
[warning] 115-115: x/evm/precompile/precompile.go#L115
Added line #L115 was not covered by testsx/evm/precompile/wasm.go
[warning] 89-89: x/evm/precompile/wasm.go#L89
Added line #L89 was not covered by tests
[warning] 94-95: x/evm/precompile/wasm.go#L94-L95
Added lines #L94 - L95 were not covered by tests
[warning] 110-110: x/evm/precompile/wasm.go#L110
Added line #L110 was not covered by tests
[warning] 113-114: x/evm/precompile/wasm.go#L113-L114
Added lines #L113 - L114 were not covered by tests
[warning] 153-153: x/evm/precompile/wasm.go#L153
Added line #L153 was not covered by tests
[warning] 188-188: x/evm/precompile/wasm.go#L188
Added line #L188 was not covered by tests
[warning] 192-192: x/evm/precompile/wasm.go#L192
Added line #L192 was not covered by tests
[warning] 196-197: x/evm/precompile/wasm.go#L196-L197
Added lines #L196 - L197 were not covered by tests
[warning] 201-201: x/evm/precompile/wasm.go#L201
Added line #L201 was not covered by tests
[warning] 234-234: x/evm/precompile/wasm.go#L234
Added line #L234 was not covered by tests
[warning] 238-238: x/evm/precompile/wasm.go#L238
Added line #L238 was not covered by tests
[warning] 244-245: x/evm/precompile/wasm.go#L244-L245
Added lines #L244 - L245 were not covered by tests
[warning] 250-250: x/evm/precompile/wasm.go#L250
Added line #L250 was not covered by tests
[warning] 256-256: x/evm/precompile/wasm.go#L256
Added line #L256 was not covered by tests
[warning] 286-286: x/evm/precompile/wasm.go#L286
Added line #L286 was not covered by tests
[warning] 290-290: x/evm/precompile/wasm.go#L290
Added line #L290 was not covered by tests
[warning] 295-296: x/evm/precompile/wasm.go#L295-L296
Added lines #L295 - L296 were not covered by tests
[warning] 304-305: x/evm/precompile/wasm.go#L304-L305
Added lines #L304 - L305 were not covered by tests
[warning] 309-312: x/evm/precompile/wasm.go#L309-L312
Added lines #L309 - L312 were not covered by tests
[warning] 316-317: x/evm/precompile/wasm.go#L316-L317
Added lines #L316 - L317 were not covered by tests
[warning] 351-351: x/evm/precompile/wasm.go#L351
Added line #L351 was not covered by tests
[warning] 355-355: x/evm/precompile/wasm.go#L355
Added line #L355 was not covered by tests
[warning] 365-366: x/evm/precompile/wasm.go#L365-L366
Added lines #L365 - L366 were not covered by tests
[warning] 372-373: x/evm/precompile/wasm.go#L372-L373
Added lines #L372 - L373 were not covered by testsx/evm/precompile/wasm_parse.go
[warning] 51-58: x/evm/precompile/wasm_parse.go#L51-L58
Added lines #L51 - L58 were not covered by tests
🔇 Additional comments (39)
x/evm/embeds/artifacts/contracts/FunToken.sol/IFunToken.json (2)
3-4: Contract and source file name updated.The contract name has been changed from
IFunTokentoFunToken, and the source file has been updated fromcontracts/IFunToken.soltocontracts/FunToken.sol. This change suggests that the contract might have been transformed from an interface to a concrete implementation.Let's verify if this change is consistent across the codebase:
#!/bin/bash # Description: Check for any remaining references to IFunToken and verify FunToken usage # Test 1: Search for any remaining IFunToken references echo "Searching for IFunToken references:" rg --type-add 'sol:*.sol' --type sol --type json 'IFunToken' # Test 2: Verify FunToken usage echo "Verifying FunToken usage:" rg --type-add 'sol:*.sol' --type sol --type json 'FunToken' # Test 3: Check if FunToken.sol exists echo "Checking for FunToken.sol file:" fd --type f 'FunToken.sol'
Line range hint
27-28: Verify the contract implementation status.The bytecode and deployedBytecode are both empty ("0x"), which is typical for interface contracts. However, the contract name has changed from
IFunTokentoFunToken, suggesting this might now be a concrete implementation. This inconsistency needs to be addressed:
- If
FunTokenis meant to be a concrete implementation, it should have non-empty bytecode.- If it's still an interface, consider keeping the "I" prefix in the name (e.g.,
IFunToken) to follow common naming conventions for interfaces.Let's verify the contract's implementation status:
Please review the contract implementation and ensure consistency between the contract name, its content, and the generated artifact.
x/evm/precompile/errors.go (6)
1-9: LGTM: Package declaration and imports are appropriate.The package name
precompileand the imported packages are suitable for the file's purpose of handling errors and validations related to Ethereum smart contract interactions.
12-14: LGTM: ErrArgTypeValidation function is well-implemented.The function provides a clear and informative error message for type validation failures, including both the expected Solidity type and the actual argument. It follows Go's error handling conventions and is concise.
17-23: LGTM: Error wrapping functions are well-implemented.Both
ErrInvalidArgsandErrMethodCalledfunctions follow Go's error wrapping conventions and provide clear, concise error messages with additional context. This approach enhances error handling and debugging capabilities.
33-45: Suggestion: Optimize weiValue check and improve test coverage.The
assertContractQueryfunction correctly prevents unintended value transfer during query operations, which is essential for maintaining the integrity of read-only contract interactions.Consider the following suggestions:
- Optimize the weiValue check:
- if weiValue != nil && weiValue.Sign() != 0 { + if weiValue != nil && !weiValue.IsZero() {This change simplifies the check and makes the intention clearer.
- Improve test coverage:
The static analysis tool indicates that lines 39-41 (the error case) are not covered by tests. To ensure robust error handling, please add a test case that covers the scenario where
weiValueis non-zero. This will improve the overall test coverage and validate the error condition.#!/bin/bash # Verify test coverage for assertContractQuery function ast-grep --lang go --pattern 'func TestAssertContractQuery($$$) { $$$ contract := &vm.Contract{$$$} contract.SetValue($$$) // Set a non-zero value err := assertContractQuery(contract) $$$ }'Overall, the function is well-implemented and provides clear error messages.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-41: x/evm/precompile/errors.go#L39-L41
Added lines #L39 - L41 were not covered by tests
1-45: Overall: Well-implemented error handling with room for improved test coverage.This new file introduces robust error handling and validation functions for Ethereum smart contract interactions. The implemented functions enhance error reporting, prevent unintended state changes, and ensure proper usage of methods. They follow Go conventions and provide clear, informative error messages.
Areas for improvement:
- Increase test coverage, particularly for error cases in
assertNotReadonlyTxandassertContractQueryfunctions.- Consider the suggested optimization for the
weiValuecheck inassertContractQuery.These improvements will further enhance the reliability and maintainability of the codebase.
To verify the overall test coverage for this file, run the following command:
#!/bin/bash # Check test coverage for x/evm/precompile/errors.go go test -coverprofile=coverage.out ./x/evm/precompile go tool cover -func=coverage.out | grep errors.goThis will help identify specific areas where test coverage can be improved.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 28-28: x/evm/precompile/errors.go#L28
Added line #L28 was not covered by tests
[warning] 39-41: x/evm/precompile/errors.go#L39-L41
Added lines #L39 - L41 were not covered by tests
26-31: LGTM with suggestion: Add test coverage for assertNotReadonlyTx.The
assertNotReadonlyTxfunction correctly prevents state changes during static calls, which is crucial for maintaining the integrity of read-only operations. The error message is clear and informative.However, the static analysis tool indicates that line 28 (the error case) is not covered by tests.
To ensure robust error handling, please add a test case that covers the scenario where
readOnlyis true andisTxis true. This will improve the overall test coverage and validate the error condition.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 28-28: x/evm/precompile/errors.go#L28
Added line #L28 was not covered by testsx/evm/embeds/README.md (2)
10-54: Excellent addition of Solidity documentation guidelines!The "Precompile Solidity Documentation" section is a valuable addition to the README. It provides comprehensive guidelines for documenting Solidity contracts, which is crucial for maintaining clear and understandable code, especially in the context of implementing precompiles for Wasm contracts.
Key strengths of this section:
- Clear explanations of various documentation tags (@notice, @param, @dev, @return, @inheritdoc, @title, @author).
- Best practices for using each tag.
- A relevant example from IHooks.sol that illustrates the proper usage of these tags.
This addition will greatly assist developers in writing well-documented and maintainable Solidity code for the Nibiru project.
1-54: Overall, these additions significantly improve the documentation.The changes to this README file are all positive additions that enhance the documentation for the Nibiru Contract Embeds. The new sections on "Hacking" and "Precompile Solidity Documentation" provide valuable information for developers working on the project, particularly in the context of implementing precompiles for Wasm contracts.
These additions:
- Improve the onboarding process for new developers with clear setup instructions.
- Establish best practices for Solidity contract documentation, which will lead to more maintainable and understandable code.
- Align well with the PR objectives by providing guidance relevant to implementing precompiles.
Great job on enhancing the documentation!
x/evm/evmtest/erc20.go (4)
13-13: LGTM: New import for Nibiru's Ethereum packageThe addition of the
"github.com/NibiruChain/nibiru/v2/eth"import is appropriate for the new functionality being added to this file.
91-101: LGTM: New AssertBankBalanceEqual functionThe new
AssertBankBalanceEqualfunction is a valuable addition to the test utilities. It correctly converts Ethereum addresses to Nibiru addresses and compares bank balances using appropriate methods. The error message is clear and informative.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-100: x/evm/evmtest/erc20.go#L97-L100
Added lines #L97 - L100 were not covered by tests
Line range hint
1-101: Summary: Enhancements to EVM testing utilitiesThe changes in this file improve the testing capabilities for EVM and bank operations, which aligns well with the PR objectives of enhancing interoperability between EVM and Wasm environments. The new
AssertBankBalanceEqualfunction provides a valuable tool for verifying bank balances in the context of EVM operations.While the implementation is sound, ensure that appropriate test coverage is added for the new function to maintain the overall reliability of the test suite.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-100: x/evm/evmtest/erc20.go#L97-L100
Added lines #L97 - L100 were not covered by tests
91-101: Consider adding test coverage for AssertBankBalanceEqualWhile the function itself is well-implemented, the static analysis tool indicates that these lines are not covered by tests.
To ensure the reliability of this test utility, consider adding unit tests for the
AssertBankBalanceEqualfunction. Here's a script to verify the current test coverage:🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-100: x/evm/evmtest/erc20.go#L97-L100
Added lines #L97 - L100 were not covered by testsx/evm/embeds/embeds.go (2)
20-23: LGTM: New precompile JSON variables addedThe addition of
funtokenPrecompileJSONandwasmPrecompileJSONvariables is consistent with the PR objectives. These new embedded JSON files will be used for the FunToken and Wasm precompile contracts, respectively.
61-61: LGTM: SmartContract_Wasm.MustLoad() added to init functionThe addition of
SmartContract_Wasm.MustLoad()to theinitfunction ensures that the new Wasm precompile contract is properly loaded during initialization. This change is consistent with the loading of other smart contracts and supports the PR objective.x/evm/precompile/funtoken_test.go (3)
26-28: LGTM! FuntokenSuite struct is well-defined.The FuntokenSuite struct is correctly implemented as a wrapper around suite.Suite. This change is part of the refactoring to separate test suites, which improves the overall structure and organization of the tests.
80-80: LGTM! TestHappyPath method signature updated correctly.The TestHappyPath method has been correctly updated to use FuntokenSuite instead of Suite. This change is consistent with the refactoring of the test suite structure and maintains the existing functionality while improving code organization.
Line range hint
1-140: Overall, the changes to funtoken_test.go are well-implemented and align with the PR objectives.The modifications to this file improve the test structure by separating FunToken and Wasm-related tests into distinct suites. This refactoring maintains existing functionality while preparing for the addition of Wasm-related tests, which aligns with the PR's goal of implementing a precompile for Wasm contract execution.
The changes are consistent, follow Go best practices, and enhance the overall organization of the test suite. Great job on improving the test structure!
x/common/testutil/testnetwork/start_node.go (1)
Line range hint
1-189: Changes align well with PR objectivesThe modifications to the
startNodeAndServersfunction, particularly the simplification of the JSON-RPC server startup process, contribute positively to the PR's goal of enhancing EVM integration and improving interoperability between EVM and Wasm environments.The changes are minimal and focused, maintaining the existing functionality for other components while streamlining the JSON-RPC server initialization. This approach ensures that the improvements are targeted and do not introduce unnecessary complexity or potential issues in other parts of the system.
x/evm/embeds/artifacts/contracts/Wasm.sol/IWasm.json (3)
1-204: LGTM: Overall structure of IWasm interfaceThe structure of the IWasm interface follows the standard Hardhat artifact format and adheres to Solidity and EVM standards. The empty bytecode fields are expected for an interface.
47-94: LGTM: executeMulti function structureThe
executeMultifunction is well-designed to allow batch execution of multiple Wasm contracts. Its structure is consistent with the singleexecutefunction, and thepayablemodifier is appropriately used. This function provides an efficient way to interact with multiple contracts in a single transaction.
1-204: Overall assessment: Well-structured interface with room for optimizationThe
IWasminterface is well-designed and provides a comprehensive set of functions for interacting with Wasm contracts from the EVM environment. It successfully addresses the PR objectives by enabling one-way EVM calls to invoke/execute Wasm contracts.Key strengths:
- Clear function definitions for execute, query, and instantiate operations.
- Proper use of
payableandviewmodifiers.- Support for batch operations with
executeMulti.Recommendations for improvement:
- Consider using
addresstype instead ofstringfor contract addresses and admin parameters across all functions. This change would improve gas efficiency and align better with Ethereum's native types.- Ensure that the implementation of these functions in the actual contract aligns with the interface and handles any necessary conversions between address types and strings if required by the underlying Wasm execution environment.
These optimizations would enhance the interface's efficiency while maintaining its functionality and alignment with the PR objectives.
CHANGELOG.md (8)
125-125: New feature added: Precompile for one-way EVM calls to Wasm contractsThis change introduces a significant new feature that allows EVM to interact with Wasm contracts. It's important to highlight this addition as it enhances the interoperability of the blockchain.
Line range hint
1-124: LGTM: Comprehensive changelog for unreleased changesThe unreleased section provides a detailed overview of recent changes, including state machine breaking changes, features, improvements, and dependency updates. The format is consistent and informative.
Line range hint
127-219: Comprehensive changelog for v1.0.0The changelog for v1.0.0 is well-structured and provides detailed information about the changes, including features, state machine breaking changes, improvements, and bug fixes. It also includes links to the relevant pull requests, which is helpful for developers who want to dive deeper into specific changes.
Line range hint
221-316: Well-documented changes for v0.21.11 to v0.19.4These sections provide clear and concise information about the changes in each version, including state machine breaking changes, features, and improvements. The format is consistent across versions, which makes it easy to read and understand the evolution of the project.
Line range hint
318-405: Detailed changelog for versions v0.16.3 to v0.15.0These sections maintain the same high quality of documentation as the previous ones, clearly outlining features, bug fixes, and improvements for each version. The inclusion of links to pull requests is particularly helpful.
Line range hint
407-519: Consistent documentation for versions v0.14.0 to v0.11.0The changelog entries for these versions continue to provide detailed information about changes, improvements, and bug fixes. The consistency in format and level of detail is commendable.
Line range hint
521-619: Well-maintained changelog for earlier versionsThe changelog entries for versions v0.10.0 down to v0.9.2 maintain the same level of detail and consistency as the later versions. This demonstrates a long-standing commitment to good documentation practices.
Line range hint
1-619: Excellent maintenance of the CHANGELOG.md fileThis CHANGELOG.md file is exemplary in its consistency, detail, and adherence to best practices. It provides a comprehensive history of the Nibiru blockchain's development, with clear categorization of changes and links to relevant pull requests. The use of Semantic Versioning and the Keep a Changelog format enhances its readability and usefulness. This level of documentation is invaluable for developers, contributors, and users of the project.
x/evm/embeds/contracts/Wasm.sol (3)
8-73: InterfaceIWasmaligns with PR objectives and is well-definedThe
IWasminterface provides necessary methods for EVM contracts to interact with Wasm contracts, fulfilling the interoperability goals outlined in the PR objectives.
17-17:⚠️ Potential issueConsider validating the format of
contractAddrwithin functionsSince
contractAddris expected to be a Bech32 address, ensure that the implementation includes validation to prevent errors due to incorrect address formats.Would you like assistance in drafting code for address validation?
9-12: EnsureBankCoinstruct is compatible withcalldataTo allow
BankCoin[] calldata funds, confirm that theBankCoinstruct members are compatible withcalldata.Run the following script to check if
BankCoinstruct can be used withcalldata:x/evm/precompile/precompile.go (2)
21-25: Imports added correctlyThe new imports for
storeandgethparamsare appropriate and necessary for the added functionality in theRequiredGasfunction.
51-51: Addition ofPrecompileWasmto custom precompiles is appropriateIncluding
PrecompileWasmin the list of custom precompile setup functions correctly registers the new precompile, enhancing the EVM's capabilities as per the PR objectives.x/evm/precompile/funtoken.go (1)
39-45:⚠️ Potential issueInconsistency between documented operations and gas cost calculation
The comments indicate that the FunToken precompile performs 3 operations (labeled 1-3), but the
RequiredGasmethod returnsgethparams.TxGas * 2, which accounts for only 2 operations. Please verify whether the gas cost should reflect all three operations or update the comments to accurately represent the operations being performed.go.mod (1)
63-64: Verify the necessity and usage of added dependenciesThe following dependencies have been added:
github.com/status-im/keycard-go v0.0.0-20190316090335-8537d3370df4golang.org/x/crypto v0.21.0Please ensure that these dependencies are necessary for implementing the precompile feature for one-way EVM calls to invoke or execute Wasm contracts. Additionally, verify that they are being utilized in the codebase and do not introduce any security or licensing issues.
You can run the following script to identify where these dependencies are used:
x/evm/precompile/wasm.go (1)
301-320: Confirm Intended Behavior on Error Handling inexecuteMultiIn the
executeMultifunction, if an error occurs during the execution of one of the messages, the function returns immediately, and subsequent messages are not executed. Please verify if this is the intended behavior. If you intend for all messages to be executed regardless of individual failures, consider modifying the error handling to collect errors and continue processing.Would you like assistance in modifying the code to handle errors per message and continue execution?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 304-305: x/evm/precompile/wasm.go#L304-L305
Added lines #L304 - L305 were not covered by tests
[warning] 309-312: x/evm/precompile/wasm.go#L309-L312
Added lines #L309 - L312 were not covered by tests
[warning] 316-317: x/evm/precompile/wasm.go#L316-L317
Added lines #L316 - L317 were not covered by tests
| WasmExecuteMsg[] memory executeMsgs | ||
| ) payable external returns (bytes[] memory responses); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize gas usage in executeMulti by using calldata
Update the executeMulti function parameters to use calldata, which is more gas-efficient for external functions.
function executeMulti(
- WasmExecuteMsg[] memory executeMsgs
+ WasmExecuteMsg[] calldata executeMsgs
) payable external returns (bytes[] memory responses);📝 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.
| WasmExecuteMsg[] memory executeMsgs | |
| ) payable external returns (bytes[] memory responses); | |
| WasmExecuteMsg[] calldata executeMsgs | |
| ) payable external returns (bytes[] memory responses); |
| if e := assertNotReadonlyTx(readOnly, true); e != nil { | ||
| err = e | ||
| return |
There was a problem hiding this comment.
Add tests to cover error handling for read-only transactions
The error handling code in bankSend (lines 125-127) for read-only transactions is not covered by tests, as indicated by the static analysis tool. To ensure robustness and prevent regressions, please add unit tests that cover this code path.
Would you like assistance in writing these tests or opening a GitHub issue to track this task?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 126-127: x/evm/precompile/funtoken.go#L126-L127
Added lines #L126 - L127 were not covered by tests
|
|
||
| FunTokenMethod_BankSend: true, | ||
| } |
There was a problem hiding this comment.
Potential Unintended Entry in precompileMethodIsTxMap
The entry FunTokenMethod_BankSend in precompileMethodIsTxMap may have been included unintentionally, as it seems unrelated to the Wasm precompile methods defined in this file.
Apply this diff to remove the unintended entry:
WasmMethod_queryRaw: false,
- FunTokenMethod_BankSend: true,
}📝 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.
| FunTokenMethod_BankSend: true, | |
| } | |
| WasmMethod_queryRaw: false, | |
| } |
| func (s *WasmSuite) assertWasmCounterStateRaw( | ||
| deps evmtest.TestDeps, | ||
| wasmContract sdk.AccAddress, | ||
| wantCount int64, | ||
| ) { | ||
| keyBz := []byte(`state`) | ||
| callArgs := []any{ | ||
| wasmContract.String(), | ||
| keyBz, | ||
| } | ||
| input, err := embeds.SmartContract_Wasm.ABI.Pack( | ||
| string(precompile.WasmMethod_queryRaw), | ||
| callArgs..., | ||
| ) | ||
| s.Require().NoError(err) | ||
|
|
||
| ethTxResp, err := deps.EvmKeeper.CallContractWithInput( | ||
| deps.Ctx, deps.Sender.EthAddr, &precompile.PrecompileAddr_Wasm, true, input, | ||
| ) | ||
| s.Require().NoError(err) | ||
| s.Require().NotEmpty(ethTxResp.Ret) | ||
|
|
||
| s.T().Log("Parse the response contract addr and response bytes") | ||
| s.T().Logf("ethTxResp.Ret: %s", ethTxResp.Ret) | ||
|
|
||
| var queryResp []byte | ||
| err = embeds.SmartContract_Wasm.ABI.UnpackIntoInterface( | ||
| &queryResp, | ||
| string(precompile.WasmMethod_queryRaw), | ||
| ethTxResp.Ret, | ||
| ) | ||
| s.Require().NoError(err) | ||
| s.T().Logf("queryResp: %s", queryResp) | ||
|
|
||
| var wasmMsg wasm.RawContractMessage | ||
| s.NoError(wasmMsg.UnmarshalJSON(queryResp), queryResp) | ||
| s.T().Logf("wasmMsg: %s", wasmMsg) | ||
| s.NoError(wasmMsg.ValidateBasic()) | ||
|
|
||
| var typedResp QueryMsgCountResp | ||
| s.NoError(json.Unmarshal(wasmMsg, &typedResp)) | ||
| s.EqualValues(wantCount, typedResp.Count) | ||
| s.EqualValues(deps.Sender.NibiruAddr.String(), typedResp.Owner) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating 'assertWasmCounterState' and 'assertWasmCounterStateRaw'
Both functions perform similar assertions on the Wasm contract state. You might refactor them into a single function with a parameter to choose between query and queryRaw, reducing code duplication.
Purpose / Abstract
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests