fix(evm-precompiles): add assertNumArgs validation #2060
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces extensive updates to the Nibiru project's changelog, detailing changes across multiple versions. Key modifications include breaking changes to the state machine, enhancements to Ethereum compatibility, and various bug fixes, particularly in the EVM context. Additionally, new methods and validation checks have been added to improve error handling and argument validation in the precompile package. The updates aim to enhance both user-facing features and internal code quality. Changes
Possibly related PRs
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
x/evm/precompile/errors.go (1)
46-55: LGTM! Consider adding an example usage in the comment.The
assertNumArgsfunction is well-implemented and provides a valuable addition to the error handling capabilities of the precompile package. The logic is correct, and the error message is clear and informative.Consider enhancing the function comment by adding an example usage. This can help other developers quickly understand how to use the function. For instance:
// assertNumArgs checks if the number of provided arguments matches the expected // count. If lenArgs does not equal wantArgsLen, it returns an error describing // the mismatch between expected and actual argument counts. +// +// Example usage: +// if err := assertNumArgs(len(args), 2); err != nil { +// return err +// } func assertNumArgs(lenArgs, wantArgsLen int) error {x/evm/precompile/wasm_parse.go (4)
87-90: LGTM! Consider adding a comment for clarity.The addition of
assertNumArgsvalidation is a good improvement, aligning with the PR objectives. It ensures that the correct number of arguments is provided before proceeding with parsing.Consider adding a brief comment explaining why 5 arguments are expected, for better code readability:
// Expect 5 arguments: admin, codeID, msgArgs, label, and funds if e := assertNumArgs(len(args), 5); e != nil { err = e return }
146-149: LGTM! Consider adding a comment for clarity.The addition of
assertNumArgsvalidation is a good improvement, aligning with the PR objectives. It ensures that the correct number of arguments is provided before proceeding with parsing.Consider adding a brief comment explaining why 3 arguments are expected, for better code readability:
// Expect 3 arguments: contractAddr, msgArgs, and funds if e := assertNumArgs(len(args), 3); e != nil { err = e return }
192-195: LGTM! Consider adding a comment for clarity.The addition of
assertNumArgsvalidation is a good improvement, aligning with the PR objectives. It ensures that the correct number of arguments is provided before proceeding with parsing.Consider adding a brief comment explaining why 2 arguments are expected, for better code readability:
// Expect 2 arguments: wasmContract and req if e := assertNumArgs(len(args), 2); e != nil { err = e return }
226-229: LGTM! Consider adding a comment for clarity.The addition of
assertNumArgsvalidation is a good improvement, aligning with the PR objectives. It ensures that the correct number of arguments is provided before proceeding with parsing.Consider adding a brief comment explaining why 1 argument is expected, for better code readability:
// Expect 1 argument: an array of execution messages if e := assertNumArgs(len(args), 1); e != nil { err = e return }x/evm/precompile/funtoken.go (1)
208-211: Approve changes with a minor suggestion for improvementThe addition of the
assertNumArgsvalidation is a good improvement that aligns with the PR objective. It enhances the robustness of the code by ensuring the correct number of arguments before proceeding with type assertions.Consider wrapping the error returned by
assertNumArgswith additional context about the function:if e := assertNumArgs(len(args), 3); e != nil { err = fmt.Errorf("decomposeBankSendArgs: %w", e) return }This will provide more informative error messages, making debugging easier.
x/evm/precompile/wasm.go (1)
358-361: Approve the addition of argument validation with a suggestion for improvement.The addition of
assertNumArgsvalidation is a good improvement to the function's robustness. It ensures that the correct number of arguments is provided before processing, which can prevent potential errors or unexpected behavior due to incorrect input.However, to improve error clarity, consider adding more context to the error message:
- if e := assertNumArgs(len(args), 2); e != nil { + if e := assertNumArgs(len(args), 2); e != nil { + e = fmt.Errorf("queryRaw: %w", e) err = e return }This change will make it clearer which function encountered the argument count mismatch, aiding in debugging and error handling.
CHANGELOG.md (1)
127-127: Consider adding more context to the new feature.The changelog entry for PR #2060 is quite brief. Consider adding a short description of what the
assertNumArgsvalidation does and why it's important. This would provide more context for users and developers.-- [#2060](https://github.com/NibiruChain/nibiru/pull/2060) - fix(evm-precompiles): add assertNumArgs validation +- [#2060](https://github.com/NibiruChain/nibiru/pull/2060) - fix(evm-precompiles): add assertNumArgs validation to ensure correct number of arguments are provided to precompile functions, improving robustness and error handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- x/evm/precompile/errors.go (1 hunks)
- x/evm/precompile/funtoken.go (1 hunks)
- x/evm/precompile/wasm.go (1 hunks)
- x/evm/precompile/wasm_parse.go (4 hunks)
🔇 Additional comments (14)
x/evm/precompile/errors.go (1)
46-55: Verify usage ofassertNumArgsin the codebaseThe new
assertNumArgsfunction looks good and enhances the error handling capabilities of the package. To ensure it's being utilized as intended, let's verify its usage across the codebase.Run the following script to check for usage of
assertNumArgs:If no usage is found, consider adding TODO comments in relevant places where
assertNumArgsshould be used, or update the PR description to clarify the intended usage of this new function.✅ Verification successful
Usage of
assertNumArgsverifiedThe
assertNumArgsfunction is correctly utilized in the following files:
x/evm/precompile/wasm_parse.gox/evm/precompile/wasm.gox/evm/precompile/funtoken.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of assertNumArgs function in the codebase # Search for function calls to assertNumArgs echo "Searching for assertNumArgs usage:" rg --type go "assertNumArgs\(" ./x/evm # If no results, check if the function is at least imported in other files if [ $? -ne 0 ]; then echo "No direct usage found. Checking for imports of the errors.go file:" rg --type go 'import\s+\([^)]*"[^"]*x/evm/precompile"[^)]*\)' ./x/evm fiLength of output: 703
x/evm/precompile/wasm_parse.go (1)
Line range hint
87-229: Overall changes look good. VerifyassertNumArgsimplementation.The consistent addition of
assertNumArgsvalidation across multiple functions improves the robustness of the code by validating input early. This aligns well with the PR objectives.Please ensure that the
assertNumArgsfunction is properly implemented and tested. Run the following script to verify its implementation:x/evm/precompile/funtoken.go (1)
Line range hint
1-238: Overall impact of changes is positive and focusedThe changes in this file are minimal and well-focused on improving input validation for the
decomposeBankSendArgsmethod. This enhancement aligns with the PR objective and improves the robustness of the FunToken precompile contract without affecting its core logic.The localized nature of the change minimizes the risk of introducing bugs while effectively addressing the intended improvement in argument validation.
x/evm/precompile/wasm.go (1)
Line range hint
1-384: Summary of changes tox/evm/precompile/wasm.goThe changes in this file are focused on improving the
queryRawfunction by adding argument validation. This aligns with the PR objective of addingassertNumArgsvalidation to EVM precompiles. The modification enhances the robustness of the function by ensuring the correct number of arguments is provided before processing.The changes are minimal and do not affect the overall structure or logic of the file. They represent a good improvement in error handling and input validation for the
queryRawfunction.CHANGELOG.md (10)
Line range hint
130-132: LGTM: Clear and concise release notes for v1.5.0.The release notes for v1.5.0 are clear and concise, providing the key information about enabling IBC CosmWasm smart contracts.
Line range hint
139-143: LGTM: Comprehensive release notes for v1.4.0.The release notes for v1.4.0 clearly state the main changes: PebbleDB support and increased wasm contract size limit. The breaking change is also clearly indicated.
Line range hint
150-159: LGTM: Well-structured release notes for v1.3.0.The release notes for v1.3.0 are well-structured, clearly separating features, bug fixes, and improvements. The addition of interchain accounts is highlighted as the main feature.
Line range hint
166-170: LGTM: Concise release notes for v1.2.0.The release notes for v1.2.0 are brief but clear, focusing on the addition of a burn method to the x/inflation module.
Line range hint
177-216: LGTM: Comprehensive release notes for v1.1.0.The release notes for v1.1.0 are detailed and well-organized, clearly separating state machine breaking changes, bug fixes, features, and improvements. The focus on inflation-related changes is evident.
Line range hint
223-231: LGTM: Clear release notes for v1.0.3.The release notes for v1.0.3 are concise and focused, highlighting a fix for IBC transactions from wasm contracts and the addition of a new CLI command.
Line range hint
238-240: LGTM: Concise release notes for v1.0.2.The release notes for v1.0.2 are brief but clear, focusing on the dependency update for cosmos-sdk.
Line range hint
247-249: LGTM: Clear release notes for v1.0.1.The release notes for v1.0.1 are concise, focusing on the dependency update for librocksdb.
Line range hint
256-269: LGTM: Comprehensive release notes for v1.0.0.The release notes for v1.0.0 provide a good overview of the major release, highlighting its significance for the mainnet genesis and listing the key modules included.
Line range hint
1-1191: Overall, the CHANGELOG.md is well-maintained and informative.The changelog follows best practices by adhering to the Keep a Changelog format and Semantic Versioning. It provides comprehensive information about changes across multiple versions, making it easy for users and developers to understand the project's evolution. The clear categorization of changes (Features, Improvements, Bug Fixes, etc.) and the use of links to pull requests enhance its usefulness.
While most entries are clear and informative, some could benefit from additional context or explanation, particularly for more technical changes. Consider adding brief descriptions of the impact or importance of changes where it's not immediately obvious.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2060 +/- ##
==========================================
- Coverage 65.13% 65.06% -0.08%
==========================================
Files 269 269
Lines 17007 17029 +22
==========================================
+ Hits 11078 11080 +2
- Misses 5000 5013 +13
- Partials 929 936 +7
|
Purpose / Abstract
The logic for
method.Inputs.Unpackdoes not validate the number of arguments the same way thatmethod.Inputs.Packdoes, so this validation check should still be present.Note that it's very difficult to actually hit these error blocks in the tests because we always ABI
PackbeforeUnpacking , which performs this validation. I think clients do this before broadcasting transactions too. The error blocks may be impossible to reach, but I'm not certain, so it's better to err on the side of more defensive programming.Summary by CodeRabbit
CHANGELOG.mdto reflect the latest changes and improvements.