refactor: general code style cleanups on precompile and statedb journal #2100
Conversation
1. Created NibiruBankKeeper with safety around NIBI transfers inside of EthereumTx. 2. The "PrecompileCalled" JournalChange now has a propery implementation and a strong test case to show that reverting the precompile calls works as intended. 3. Remove unneeded functions created for testing with low-level struct fields.
WalkthroughThe changes in this pull request involve significant updates to the Nibiru project, particularly the Nibiru EVM and its modules. Key modifications include enhancements to ERC20 metadata handling, adjustments to gas fee calculations, and improvements in state consistency during precompile execution. New features such as foundry support for EVM development and fun token creation fee validation are introduced. The codebase also undergoes refactoring for clarity and efficiency, alongside comprehensive testing enhancements and dependency updates to ensure compatibility. Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2100 +/- ##
==========================================
- Coverage 64.58% 64.57% -0.01%
==========================================
Files 271 271
Lines 21237 21223 -14
==========================================
- Hits 13716 13705 -11
+ Misses 6574 6569 -5
- Partials 947 949 +2
|
![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
🧹 Outside diff range and nitpick comments (11)
x/evm/precompile/wasm_parse.go (1)
Line range hint 209-246: Consider extracting the message struct type.
The nested struct definition could be moved to a separate type for better reusability and maintainability.
Consider extracting the struct definition:
// WasmExecuteMessage represents a single contract execution message
type WasmExecuteMessage struct {
ContractAddr string `json:"contractAddr"`
MsgArgs []byte `json:"msgArgs"`
Funds []BankCoin `json:"funds"`
}
func (p precompileWasm) parseExecuteMultiArgs(args []interface{}) (wasmExecMsgs []WasmExecuteMessage, err error) {
// ... rest of the function
}x/evm/precompile/precompile.go (1)
Line range hint 112-133: Consider enhancing the panic prevention comment.
While the code is correct, the comment about map panic prevention could be more explicit about how the unit tests verify this safety.
Consider updating the comment to:
-// Map access could panic. We know that it won't panic because all methods
-// are in the map, which is verified by unit tests.
+// Map access is safe as unit tests verify that all PrecompileMethods defined
+// in the ABI are present in the isMutation map.x/evm/statedb/journal_test.go (1)
34-35: LGTM! Consider using test logger consistently.
The variable rename to helloWorldCounterWasm improves clarity by better describing the contract's purpose.
Consider using s.T().Logf instead of fmt.Printf for consistency with other logging in the test:
-fmt.Printf("wasmContract: %s\n", helloWorldCounterWasm)
+s.T().Logf("wasmContract: %s", helloWorldCounterWasm)x/evm/statedb/statedb.go (1)
606-606: Document the rationale for maxMultistoreCacheCount value.
Consider adding a comment explaining why 10 was chosen as the maximum number of precompile calls per transaction. This helps future maintainers understand if this limit can be safely adjusted.
-const maxMultistoreCacheCount uint8 = 10
+// maxMultistoreCacheCount limits the number of Nibiru-specific precompiled contract
+// calls within a single transaction to prevent excessive state caching and potential
+// resource exhaustion.
+const maxMultistoreCacheCount uint8 = 10CHANGELOG.md (2)
66-68: Consider standardizing the formatting for consistency.
The issue references could be made more consistent with the rest of the changelog by:
- Using the shorter issue reference format like other entries
- Removing the indentation
- Combining into a single line with comma separation
- - <https://github.com/NibiruChain/nibiru/issues/2083>
- - <https://github.com/code-423n4/2024-10-nibiru-zenith/issues/43>
- - <https://github.com/code-423n4/2024-10-nibiru-zenith/issues/47>
+- #2083, code-423n4/2024-10-nibiru-zenith#43, code-423n4/2024-10-nibiru-zenith#47Line range hint 1-161: Consider reorganizing the unreleased section for better readability.
The unreleased section has grown quite large. Consider:
- Adding subsections within major categories to group related changes
- Using consistent formatting for PR references
- Adding dates to section headers where applicable
x/evm/precompile/wasm.go (5)
Line range hint 158-167: Replace 'start' with 'startResult' in function 'query'
In the query function, start is used instead of startResult when assigning method, args, ctx. This could cause errors due to start being undefined. Please replace start with startResult to ensure correct functionality.
Apply this diff:
-method, args, ctx := start.Method, start.Args, start.CacheCtx
+method, args, ctx := startResult.Method, startResult.Args, startResult.CacheCtxLine range hint 319-330: Replace 'start' with 'startResult' in function 'queryRaw'
In queryRaw, you're assigning method, args, ctx from start instead of startResult. This could lead to using undefined variables. Please update start to startResult to align with the changes made.
Apply this diff:
-method, args, ctx := start.Method, start.Args, start.CacheCtx
+method, args, ctx := startResult.Method, startResult.Args, startResult.CacheCtx127-127: Consider defining error messages as constants for consistency
The error message "wasm execute cannot be called in read-only mode" is hardcoded. For better maintainability and consistency across the codebase, consider defining this error message as a constant or using a predefined error variable.
211-211: Consider defining error messages as constants for consistency
The error message "wasm instantiate cannot be called in read-only mode" is hardcoded. To improve consistency, consider defining this message as a constant or reusing an existing error variable.
262-262: Consider defining error messages as constants for consistency
The error message "wasm executeMulti cannot be called in read-only mode" is hardcoded. Defining this error message as a constant can enhance code clarity and ease future modifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- CHANGELOG.md (2 hunks)
- x/evm/precompile/.gitignore (0 hunks)
- x/evm/precompile/errors.go (0 hunks)
- x/evm/precompile/funtoken.go (3 hunks)
- x/evm/precompile/oracle.go (4 hunks)
- x/evm/precompile/precompile.go (5 hunks)
- x/evm/precompile/test/export.go (9 hunks)
- x/evm/precompile/wasm.go (9 hunks)
- x/evm/precompile/wasm_parse.go (9 hunks)
- x/evm/statedb/journal_test.go (6 hunks)
- x/evm/statedb/statedb.go (2 hunks)
💤 Files with no reviewable changes (2)
- x/evm/precompile/.gitignore
- x/evm/precompile/errors.go
🔇 Additional comments (30)
x/evm/precompile/oracle.go (4)
27-27: LGTM: Consistent method naming.
The change from RequiredGas to requiredGas aligns with Go naming conventions for internal helper functions.
41-45: LGTM: Improved clarity in variable naming and parameter passing.
The changes enhance readability by:
- Using a more descriptive variable name
startResult - Passing only the required
contract.Inputinstead of the entire contract
Line range hint 88-102: LGTM: Clear and robust argument parsing.
The new parseQueryExchangeRateArgs method:
- Has a more accurate name reflecting its purpose
- Maintains thorough error handling
- Follows single responsibility principle
49-49: Verify consistent removal of readonly parameter.
The removal of the readonly parameter appears correct, but let's verify this change is consistent across other precompile implementations.
✅ Verification successful
Removal of readonly parameter is consistent with codebase
Based on the search results:
- The
queryExchangeRatemethod definition and implementation inoracle.godoes not use the readonly parameter - The method is correctly defined in the Solidity interface (
IOracle.sol) as a view function - All precompile Run methods (
oracle.go,wasm.go,funtoken.go) consistently have the readonly parameter in their signatures, but it's not being passed down to internal methods - Test files show consistent usage without readonly parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining queryExchangeRate method calls that might still use the readonly parameter
rg -A 2 "queryExchangeRate.*readonly"
# Search for other precompile Run methods to verify consistent parameter usage
ast-grep --pattern 'func \($_\) Run\($_, \$_, $_\) \($_, $_\)'
Length of output: 104
Script:
#!/bin/bash
# Search for queryExchangeRate method definition and calls
rg -A 5 "queryExchangeRate"
# Search for all Run methods in precompile files to verify parameter consistency
fd precompile --exec rg -A 5 "func.*Run"
# Search for any test files that might use queryExchangeRate
fd "_test" --exec rg "queryExchangeRate"
Length of output: 7023
x/evm/precompile/wasm_parse.go (4)
24-50: LGTM! Improved robustness in funds parsing.
The changes enhance safety through:
- Early nil handling
- Explicit type assertions
- Panic-safe coin construction
- Clear documentation of design decisions
70-70: LGTM! Consistent type usage.
The signature change aligns with the codebase-wide standardization to interface{}.
Line range hint 127-174: LGTM! Enhanced error handling and code organization.
The changes improve:
- Error message clarity
- Variable naming
- Code readability
195-199: LGTM! Added safety check for request bytes.
The explicit type assertion check prevents potential runtime issues.
x/evm/precompile/funtoken.go (3)
35-35: LGTM: Simplified gas calculation
The direct use of embeds.SmartContract_FunToken.ABI improves code clarity and reduces indirection.
Line range hint 49-68: LGTM: Well-structured Run method implementation
The implementation shows good practices:
- Uses OnRunStart for consistent input handling
- Clear error messages
- Proper method dispatch with exhaustive error handling
98-100: Verify thread safety after mutex removal
The removal of the mutex simplifies the code, but we should verify that this doesn't introduce race conditions. Please confirm that:
- The state modifications in bankSend are atomic
- The SDK's context handling provides sufficient thread safety
Let's check for other concurrent access patterns in the codebase:
✅ Verification successful
Thread safety is maintained through SDK's state management
The mutex removal is safe because:
- The
bankSendmethod operates within Cosmos SDK's context (sdk.Context) and state management system (StateDB), which provides transaction isolation - All state modifications are atomic through the SDK's
StateDBcommit mechanism and journal entries - The precompile execution flow uses
CacheCtxForPrecompile()andCommitCacheCtx()to ensure state changes are properly isolated and committed - No concurrent access patterns were found in the codebase that would require additional synchronization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for concurrent access patterns to bankSend or related methods
rg -A 5 "bankSend.*go.*routine" || \
rg -A 5 "go func.*bankSend"
# Check for other mutex usage patterns in precompile
rg -A 5 "sync\.Mutex|sync\.RWMutex" "x/evm/precompile/"
Length of output: 119
Script:
#!/bin/bash
# Let's analyze the bankSend implementation and its dependencies
ast-grep --pattern 'func $_ bankSend($$$) {
$$$
}'
# Check for any concurrent access patterns in the precompile package
rg -B 2 -A 5 "bankSend" "x/evm/precompile/"
# Look for any SDK context usage patterns
rg -B 2 -A 5 "sdk\.Context" "x/evm/precompile/"
# Check for any other state modifications in the precompile
rg -B 2 -A 5 "StateDB|stateDB" "x/evm/precompile/"
Length of output: 10470
x/evm/precompile/precompile.go (5)
88-96: LGTM: Improved error handling and naming convention.
The changes improve code quality by:
- Following Go naming conventions with unexported function
- Using
collections.HumanizeBytesfor better error message readability
136-145: LGTM: Improved type safety and documentation.
The changes enhance code clarity through:
- Type-safe
PrecompileMethoddefinition - More descriptive field naming with
CacheCtx - Clear documentation of the cached context's purpose
198-210: LGTM: Robust state management implementation.
The changes properly handle state management through:
- Appropriate context caching
- Comprehensive error handling for journal changes
- Consistent struct initialization
214-214: LGTM: Clear method classification.
The renamed map provides a clear and concise way to classify precompile methods based on their state mutation behavior.
182-184: Verify the impact of the signature change.
The function signature change from *vm.Contract to []byte is a breaking change.
Let's verify all callers have been updated:
✅ Verification successful
Let me analyze the results. I can see that there are three actual callers of OnRunStart in:
x/evm/precompile/oracle.gox/evm/precompile/funtoken.gox/evm/precompile/wasm.go
And all of them are already using contract.Input which is of type []byte, so they are aligned with the new signature.
The fourth match is just a commented-out old usage in precompile.go itself showing the previous signature.
I can generate the final response now:
Breaking change in OnRunStart signature has been properly handled
All callers have been updated to use the new signature with []byte instead of *vm.Contract. The change is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all OnRunStart function calls
rg -A 2 "OnRunStart\("
Length of output: 1026
x/evm/statedb/journal_test.go (5)
39-39: LGTM! Consistent variable usage.
The assertion correctly uses the renamed variable.
131-131: LGTM! Consistent variable usage in test methods.
The increment and assertion calls correctly use the renamed variable.
Also applies to: 135-135
147-147: LGTM! Consistent variable usage.
The increment call correctly uses the renamed variable.
160-160: LGTM! Well-documented state assertions.
The assertions consistently use the renamed variable. The mathematical expressions in the expected values (e.g., 7+5+50) and accompanying comments provide clear documentation of how the expected values are calculated.
Also applies to: 168-168, 175-175, 187-187, 203-203, 211-211, 217-217
Line range hint 34-217: Great test structure and coverage!
The test is well-organized with:
- Clear sections with descriptive logging
- Comprehensive coverage of StateDB journal operations
- Good mix of success and error cases
- Proper validation of state transitions
x/evm/precompile/test/export.go (6)
14-14: LGTM: Import additions are appropriate.
The added imports for go-ethereum/core/vm and server/config are necessary for the functionality in the code.
Also applies to: 18-18
31-37: LGTM: Code readability improvements.
The changes enhance code clarity through:
- More descriptive variable names (otherArgs → instantiateArgs)
- Consistent naming (InstMsg → InstantiateMsg)
- More idiomatic Go types (any → interface{})
Also applies to: 40-41, 45-45, 52-53, 59-59, 66-66, 73-73, 75-76
102-102: LGTM: Consistent variable naming.
The variable rename from pathToWasmBin to binPath and corresponding error message updates maintain consistency and clarity.
Also applies to: 113-114, 117-117, 134-134
160-160: LGTM: Type consistency and improved documentation.
The changes maintain type consistency (any → interface{}) and add helpful documentation about UnpackIntoInterface usage.
Also applies to: 184-184
286-286: LGTM: Consistent type usage.
The change maintains type consistency with the rest of the codebase.
341-341: LGTM: Consistent type usage.
The change maintains type consistency with the rest of the codebase.
x/evm/statedb/statedb.go (2)
Line range hint 589-604: LGTM! Method changes are consistent.
The simplified method signature aligns with the changes in CacheCtxForPrecompile. The error handling for maximum precompile calls is well implemented.
Line range hint 575-587: LGTM! Verify the removal of precompileAddr parameter.
The simplified method signature and implementation look correct. The lazy initialization pattern for context caching is well implemented, ensuring proper state isolation.
Let's verify that the removed precompileAddr parameter wasn't used elsewhere:
✅ Verification successful
Let me search for any direct usage of the precompileAddr parameter in the previous implementation of CacheCtxForPrecompile:
Let me check the PrecompileCalled struct definition to confirm the complete removal of precompileAddr:
Based on the git history and struct definition, I can now provide a final response:
Removal of precompileAddr parameter is safe and correct
The parameter removal is well-justified as:
- The
precompileAddrwas previously stored inPrecompileCalled.Precompilefield which has been removed - The address was not used in the caching logic itself
- The method's core functionality of context caching and state isolation remains intact
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to precompileAddr in precompile-related code
rg -i "precompileAddr"
Length of output: 2118
Script:
#!/bin/bash
# Search for git history of the file to see the previous implementation
git log -p x/evm/statedb/statedb.go | grep -A 20 "func.*CacheCtxForPrecompile"
Length of output: 4849
Script:
#!/bin/bash
# Search for PrecompileCalled struct definition
rg -A 10 "type PrecompileCalled struct"
Length of output: 722
x/evm/precompile/wasm.go (1)
77-77: Ensure 'requiredGas' function is accessible and correctly implemented
The RequiredGas method calls requiredGas(input, embeds.SmartContract_Wasm.ABI). Please verify that the requiredGas function is defined within the scope and correctly implements the gas calculation logic. If it's an unexported function, confirm that it's intended and that all necessary dependencies are met.
x/evm/precompile/funtoken.go
Outdated
| @@ -242,7 +227,7 @@ | |||
| return nil | |||
| } | |||
|
|
|||
| func (p precompileFunToken) decomposeBankSendArgs(args []any) ( | |||
| func (p precompileFunToken) decomposeBankSendArgs(args []interface{}) ( | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using modern Go type syntax
The method signature uses []interface{} which is the pre-Go 1.18 syntax. Consider using the more modern []any type alias introduced in Go 1.18.
-func (p precompileFunToken) decomposeBankSendArgs(args []interface{}) (
+func (p precompileFunToken) decomposeBankSendArgs(args []any) (📝 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.
| func (p precompileFunToken) decomposeBankSendArgs(args []interface{}) ( | |
| func (p precompileFunToken) decomposeBankSendArgs(args []any) ( |
x/evm/precompile/wasm.go
Outdated
| method, args, ctx := start.Method, start.Args, start.CacheCtx | ||
| defer func() { | ||
| if err != nil { | ||
| err = ErrMethodCalled(method, err) | ||
| } | ||
| }() | ||
|
|
||
| if err := assertNotReadonlyTx(readOnly, true); err != nil { | ||
| return bz, err | ||
| if readOnly { | ||
| return nil, errors.New("wasm execute cannot be called in read-only mode") | ||
| } | ||
| wasmContract, msgArgs, funds, err := p.parseExecuteArgs(args) | ||
|
|
||
| wasmContract, msgArgsBz, funds, err := p.parseExecuteArgs(args) |
There was a problem hiding this comment.
Replace 'start' with 'startResult' in function 'execute'
In the execute function, you're using start instead of startResult when assigning method, args, ctx. This may lead to undefined variable errors or incorrect behavior since start is not defined in this scope. Please update start to startResult to match the variable returned by OnRunStart.
Apply this diff to fix the issue:
-method, args, ctx := start.Method, start.Args, start.CacheCtx
+method, args, ctx := startResult.Method, startResult.Args, startResult.CacheCtxCommittable suggestion was skipped due to low confidence.
x/evm/precompile/wasm.go
Outdated
| method, args, ctx := start.Method, start.Args, start.CacheCtx | ||
| defer func() { | ||
| if err != nil { | ||
| err = ErrMethodCalled(method, err) | ||
| } | ||
| }() | ||
| if err := assertNotReadonlyTx(readOnly, true); err != nil { | ||
| return bz, err | ||
| if readOnly { | ||
| return nil, errors.New("wasm instantiate cannot be called in read-only mode") |
There was a problem hiding this comment.
Replace 'start' with 'startResult' in function 'instantiate'
In the instantiate function, the variables method, args, ctx are assigned from start instead of startResult. This can lead to undefined behavior. Please update start to startResult.
Apply this diff:
-method, args, ctx := start.Method, start.Args, start.CacheCtx
+method, args, ctx := startResult.Method, startResult.Args, startResult.CacheCtxCommittable suggestion was skipped due to low confidence.
x/evm/precompile/wasm.go
Outdated
| method, args, ctx := start.Method, start.Args, start.CacheCtx | ||
| defer func() { | ||
| if err != nil { | ||
| err = ErrMethodCalled(method, err) | ||
| } | ||
| }() | ||
| if err := assertNotReadonlyTx(readOnly, true); err != nil { | ||
| return bz, err | ||
| if readOnly { | ||
| return nil, errors.New("wasm executeMulti cannot be called in read-only mode") |
There was a problem hiding this comment.
Replace 'start' with 'startResult' in function 'executeMulti'
The executeMulti function uses start instead of startResult when assigning method, args, ctx. This inconsistency may result in errors. Please correct it by replacing start with startResult.
Apply this diff:
-method, args, ctx := start.Method, start.Args, start.CacheCtx
+method, args, ctx := startResult.Method, startResult.Args, startResult.CacheCtxCommittable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (12)
x/evm/precompile/oracle.go (3)
41-45: LGTM: Improved input handling and variable naming
The changes enhance code clarity by:
- Passing only the required contract.Input instead of the entire contract
- Using a more descriptive variable name 'startResult'
Consider adding error context to provide more detailed error information.
startResult, err := OnRunStart(evm, contract.Input, embeds.SmartContract_Oracle.ABI)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("oracle precompile OnRunStart failed: %w", err)
}49-49: LGTM: Simplified method signature
The removal of the readonly parameter is appropriate as exchange rate queries are inherently read-only operations.
Consider adding a comment above the queryExchangeRate method to document its read-only nature:
// queryExchangeRate performs a read-only query of the exchange rate for a given asset pair71-71: LGTM: Improved naming and type safety
The changes enhance code quality through:
- More accurate function naming with 'parse' instead of 'decompose'
- More idiomatic Go type usage with
[]interface{}
Consider enhancing error messages to include the actual value received:
pair, ok := args[0].(string)
if !ok {
- err = ErrArgTypeValidation("string pair", args[0])
+ err = ErrArgTypeValidation("string pair", fmt.Sprintf("%T: %v", args[0], args[0]))
return
}Also applies to: 88-104
x/evm/precompile/wasm_parse.go (3)
34-35: Enhance error message clarity.
Consider making the error message more descriptive by including the actual type received.
- return funds, ErrArgTypeValidation("BankCoin[] funds", arg)
+ return funds, ErrArgTypeValidation(fmt.Sprintf("BankCoin[] funds (got %T)", arg), arg)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 35-35: x/evm/precompile/wasm_parse.go#L35
Added line #L35 was not covered by tests
Line range hint 209-220: Consider extracting nested types for better readability.
The nested struct definitions make the code harder to maintain. Consider extracting them into named types.
// Define at package level
type WasmExecFunds struct {
Denom string `json:"denom"`
Amount *big.Int `json:"amount"`
}
type WasmExecMessage struct {
ContractAddr string `json:"contractAddr"`
MsgArgs []byte `json:"msgArgs"`
Funds []WasmExecFunds `json:"funds"`
}
// Update function signature
func (p precompileWasm) parseExecuteMultiArgs(args []interface{}) ([]WasmExecMessage, error)Line range hint 238-239: Fix incorrect error message.
The error message refers to "BankCoin[] funds" but this is actually validating the execute message array.
- err = ErrArgTypeValidation("BankCoin[] funds", arg)
+ err = ErrArgTypeValidation("WasmExecMessage[] messages", arg)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-27: x/evm/precompile/wasm_parse.go#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 35-35: x/evm/precompile/wasm_parse.go#L35
Added line #L35 was not covered by tests
x/evm/precompile/precompile.go (1)
93-93: Add test coverage for error handling.
This error path for invalid input length is not covered by tests.
Would you like me to help generate test cases for this error condition?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 93-93: x/evm/precompile/precompile.go#L93
Added line #L93 was not covered by tests
x/evm/precompile/test/export.go (3)
73-76: Add nil check for unpacked values.
While the unpacking logic is cleaner, consider adding a nil check before type assertion to prevent potential panics.
- contractAddr, err := sdk.AccAddressFromBech32(vals[0].(string))
+ if len(vals) == 0 || vals[0] == nil {
+ return nil, fmt.Errorf("empty response from contract instantiation")
+ }
+ addrStr, ok := vals[0].(string)
+ if !ok {
+ return nil, fmt.Errorf("expected string address, got %T", vals[0])
+ }
+ contractAddr, err := sdk.AccAddressFromBech32(addrStr)286-286: Consider using any instead of interface{}.
Since Go 1.18, any is the preferred alias for interface{}. Consider updating for consistency with modern Go conventions.
- callArgs := []interface{}{
+ callArgs := []any{Also applies to: 341-341
Line range hint 246-357: Consider reducing code duplication in increment functions.
IncrementWasmCounterWithExecuteMulti and IncrementWasmCounterWithExecuteMultiViaVMCall share significant code. Consider extracting the common setup logic into a helper function.
func prepareIncrementExecuteMsgs(
s *suite.Suite,
wasmContract sdk.AccAddress,
times uint,
) ([]interface{}, error) {
msgArgsBz := []byte(`{ "increment": {} }`)
var funds []precompile.WasmBankCoin
fundsJson, err := json.Marshal(funds)
if err != nil {
return nil, fmt.Errorf("marshal funds: %w", err)
}
executeMsgs := []struct {
ContractAddr string `json:"contractAddr"`
MsgArgs []byte `json:"msgArgs"`
Funds []precompile.WasmBankCoin `json:"funds"`
}{
{wasmContract.String(), msgArgsBz, funds},
}
if times == 0 {
executeMsgs = executeMsgs[:0]
} else {
for i := uint(1); i < times; i++ {
executeMsgs = append(executeMsgs, executeMsgs[0])
}
}
return []interface{}{executeMsgs}, nil
}CHANGELOG.md (1)
161-161: Consider adding more details about the cleanup changes.
The entry "refactor: cleanup statedb and precompile sections" is quite vague. Consider expanding it to briefly describe:
- What specific aspects were cleaned up
- Any notable improvements or changes made
- Impact on the codebase
x/evm/precompile/wasm.go (1)
4-4: Remove unnecessary import if not used.
The "errors" package has been added to the imports at line 4. If there are no usages of the errors package elsewhere in the code, consider removing this import to keep the code clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- CHANGELOG.md (2 hunks)
- x/evm/precompile/.gitignore (0 hunks)
- x/evm/precompile/errors.go (0 hunks)
- x/evm/precompile/funtoken.go (3 hunks)
- x/evm/precompile/oracle.go (4 hunks)
- x/evm/precompile/precompile.go (5 hunks)
- x/evm/precompile/test/export.go (9 hunks)
- x/evm/precompile/wasm.go (9 hunks)
- x/evm/precompile/wasm_parse.go (9 hunks)
- x/evm/statedb/journal_test.go (6 hunks)
- x/evm/statedb/statedb.go (2 hunks)
💤 Files with no reviewable changes (2)
- x/evm/precompile/.gitignore
- x/evm/precompile/errors.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/evm/precompile/funtoken.go
[warning] 100-100: x/evm/precompile/funtoken.go#L100
Added line #L100 was not covered by tests
x/evm/precompile/precompile.go
[warning] 93-93: x/evm/precompile/precompile.go#L93
Added line #L93 was not covered by tests
x/evm/precompile/wasm.go
[warning] 58-58: x/evm/precompile/wasm.go#L58
Added line #L58 was not covered by tests
[warning] 127-127: x/evm/precompile/wasm.go#L127
Added line #L127 was not covered by tests
[warning] 211-211: x/evm/precompile/wasm.go#L211
Added line #L211 was not covered by tests
[warning] 262-262: x/evm/precompile/wasm.go#L262
Added line #L262 was not covered by tests
x/evm/precompile/wasm_parse.go
[warning] 26-27: x/evm/precompile/wasm_parse.go#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 35-35: x/evm/precompile/wasm_parse.go#L35
Added line #L35 was not covered by tests
[warning] 197-199: x/evm/precompile/wasm_parse.go#L197-L199
Added lines #L197 - L199 were not covered by tests
x/evm/statedb/statedb.go
[warning] 602-603: x/evm/statedb/statedb.go#L602-L603
Added lines #L602 - L603 were not covered by tests
🔇 Additional comments (28)
x/evm/precompile/oracle.go (1)
27-27: LGTM: Consistent with codebase-wide refactoring
The change from RequiredGas to requiredGas aligns with the broader refactoring effort to make gas calculation functions private.
x/evm/precompile/wasm_parse.go (1)
Line range hint 153-164: LGTM! Good improvements to message validation.
The addition of message validation using ValidateBasic() improves security by ensuring message integrity before execution.
x/evm/precompile/funtoken.go (4)
35-35: LGTM: Simplified ABI access
The direct usage of embeds.SmartContract_FunToken.ABI improves code clarity by removing unnecessary indirection.
49-49: LGTM: Consistent ABI usage pattern
The change maintains consistency with the simplified ABI access pattern used throughout the file.
230-230: Verify type usage consistency
The change from []any to []interface{} should be consistent across the codebase.
#!/bin/bash
# Check for mixed usage of 'any' and 'interface{}' types
echo "Searching for 'any' type usage:"
rg "\[\]any"
echo "Searching for 'interface{}' slice type usage:"
rg "\[\]interface{}" 98-98: Verify the implications of using CacheCtx
The change from start.Ctx to start.CacheCtx suggests operations are now performed in a cached context. This could affect state management and transaction atomicity.
✅ Verification successful
Using CacheCtx is the correct implementation
The change from start.Ctx to start.CacheCtx is intentional and correct. The CacheCtx is specifically designed for precompile execution to provide isolated state operations that can be reverted by the EVM's StateDB if needed. This is evident from:
- The
OnRunStartResultstruct documentation explicitly states thatCacheCtxis "a cached SDK context that allows isolated state operations to occur that can be reverted by the EVM's [statedb.StateDB]" - The
OnRunStartfunction properly initializes this cached context throughstateDB.CacheCtxForPrecompile() - The implementation follows the documented pattern for precompile execution as shown in the example usage comments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of CacheCtx to understand the pattern
rg -A 2 "CacheCtx"
Length of output: 2267
Script:
#!/bin/bash
# Let's search for the file and its context to understand the CacheCtx usage
fd "funtoken.go" --exec cat {}
# Also search for any other files that might define or use CacheCtx
rg -l "type.*CacheCtx|CacheCtx.*struct"
Length of output: 7358
Script:
#!/bin/bash
# Let's search for the OnRunStartResult type definition and CacheCtx field
rg -A 5 "type OnRunStartResult"
# Also search for OnRunStart function to see how CacheCtx is initialized
rg -A 10 "func OnRunStart"
Length of output: 1133
Script:
#!/bin/bash
# Let's see the complete OnRunStartResult struct and OnRunStart function
cat x/evm/precompile/precompile.go
Length of output: 7249
x/evm/precompile/precompile.go (5)
Line range hint 112-134: LGTM! Improved gas calculation implementation.
The changes improve efficiency by:
- Directly using
methodByIdinstead of full input decomposition - Clear separation of mutation vs read costs
- More precise gas calculation based on actual input size
136-145: LGTM! Improved type definitions and field naming.
Good improvements:
- Type-safe
PrecompileMethodstring alias - More explicit
[]interface{}type - More descriptive
CacheCtxfield name
Line range hint 182-212: LGTM! Improved function signature and documentation.
Excellent improvements:
- Simplified interface by directly accepting contract input
- Clear documentation with usage examples
- Robust state management with proper journaling
Line range hint 214-224: LGTM! Clear categorization of precompile methods.
Good improvements:
- Concise and clear variable name
- Well-organized grouping of methods by type (mutation vs query)
Line range hint 88-107: LGTM! Clean refactoring of the function.
The unexport of decomposeInput and the simplified error message construction are good improvements.
Let's verify there are no external usages of this function since it's now unexported:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 93-93: x/evm/precompile/precompile.go#L93
Added line #L93 was not covered by tests
x/evm/statedb/journal_test.go (6)
34-35: LGTM! Improved variable naming.
The rename from wasmContract to helloWorldCounterWasm better describes the contract's purpose and improves code readability.
39-39: LGTM! Consistent variable usage.
The parameter update correctly reflects the variable rename.
131-135: LGTM! Consistent variable usage in test assertions.
The parameter updates correctly reflect the variable rename in both function calls.
147-147: LGTM! Consistent variable usage.
The parameter update correctly reflects the variable rename.
160-168: LGTM! Well-documented state transitions.
The parameter updates correctly reflect the variable rename, and the comments clearly document the expected counter values and their calculations.
Line range hint 175-217: LGTM! Thorough test documentation and consistent variable usage.
The changes maintain consistency with the variable rename while preserving the detailed documentation of snapshot and revert behavior. The assertions properly verify the state transitions at each step.
x/evm/precompile/test/export.go (3)
14-14: LGTM: Import additions are properly organized and necessary.
The added imports for vm and serverconfig packages are required for EVM operations and gas limit configuration respectively.
Also applies to: 18-18
31-41: LGTM: Improved struct naming for better clarity.
The renaming of the struct and its fields better reflects their purpose and follows Go naming conventions.
113-117: LGTM: Improved error message for better debugging.
The enhanced error message now includes both path variables, making it easier to debug file reading issues.
x/evm/statedb/statedb.go (2)
Line range hint 575-585: LGTM! API simplification improves usability.
The removal of the unused precompileAddr parameter simplifies the API while maintaining proper state isolation through the SDK's context caching mechanism.
Line range hint 575-603: Verify precompile execution flow with removed address parameter.
The removal of precompileAddr parameter from both methods represents a significant change in how precompiles are handled. Let's verify that all precompile implementations have been updated accordingly.
✅ Verification successful
Based on the search results, I can now provide a definitive response about the precompile execution flow changes:
Precompile parameter removal is safe and consistent with implementation.
The codebase scan shows that:
- The only usage of
CacheCtxForPrecompileandSavePrecompileCalledJournalChangeis inx/evm/precompile/precompile.gowhere they are called without any address parameter - All precompile implementations (
precompileWasm,precompileOracle,precompileFunToken) use the standard pattern of calling these methods through the commonOnRunStarthelper - The precompile address is already available in the
StateDBthrough other means (journal entries, contract context) - Test coverage confirms the methods work correctly without the address parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of the old method signatures or patterns
# that might expect the precompileAddr parameter
# Check for old method calls
rg -l "CacheCtxForPrecompile\s*\(\s*[^)]+\)"
# Check for precompile implementations that might expect the address
rg -l "type\s+\w+Precompile\s+struct" | while read -r file; do
echo "=== $file ==="
rg "CacheCtxForPrecompile|SavePrecompileCalledJournalChange" "$file" -C 2
done
Length of output: 101
Script:
#!/bin/bash
# Let's try a broader search to find precompile-related code and their interactions
# Search for precompile-related files and their content
rg -l "precompile" --type go
# Search for any direct usage of these methods
rg "CacheCtxForPrecompile|SavePrecompileCalledJournalChange" -A 2 --type go
# Look for struct definitions and interfaces related to precompiles
ast-grep --pattern 'type $_ struct {
$$$
}' | rg -i "precompile"
# Check for method implementations
ast-grep --pattern 'func ($_ $_) $_($$$) $$$' | rg -i "precompile"
Length of output: 70198
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 602-603: x/evm/statedb/statedb.go#L602-L603
Added lines #L602 - L603 were not covered by tests
CHANGELOG.md (1)
66-68: LGTM! Well-documented issue references.
The changelog entries properly document the fixes with clear references to the relevant GitHub issues, providing good context about the state consistency improvements in precompile execution.
x/evm/precompile/wasm.go (5)
77-77: Verify the accuracy of gas calculation with requiredGas function.
The RequiredGas method now delegates gas calculation to the requiredGas function. Please ensure that this change accurately computes the required gas for contract executions and maintains consistency with the gas cost assumptions throughout the system.
120-120: Review the use of start.CacheCtx instead of start.Ctx.
The context in the execute method has been changed to start.CacheCtx. Please verify that using CacheCtx maintains the intended state and does not introduce unintended side effects during contract execution.
158-158: Review the context change to start.CacheCtx in query method.
In the query method, the context has been updated to start.CacheCtx. Ensure that this change preserves the correct query behavior and state consistency.
204-204: Confirm context update in instantiate method.
The instantiate method now uses start.CacheCtx as the context. Please check that this change aligns with the desired context management and does not affect contract instantiation logic.
255-255: Ensure consistency with context usage in executeMulti method.
The executeMulti method utilizes start.CacheCtx for context. Verify that this adjustment is appropriate and that it maintains the correct execution flow for multiple contract calls.
| if arg == nil { | ||
| return funds, nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage needed for nil funds handling in parseFundsArg
The nil check is a valid defensive programming practice, but there are no dedicated test cases covering this scenario. The function is used in multiple places (parseExecuteArgs and parseInstantiateArgs), but the nil path specifically needs test coverage to ensure robust behavior.
- Add test cases in a new
wasm_parse_test.gofile to cover:- Passing nil argument to parseFundsArg
- Verify it returns empty coins without error
🔗 Analysis chain
Add test coverage for nil argument handling.
Good addition of the nil check for defensive programming. However, this code path lacks test coverage.
Let's verify the test coverage:
Would you like me to help generate test cases for this scenario?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any tests for parseFundsArg nil case
rg -l "TestParseFundsArg.*nil" --type go
Length of output: 42
Script:
#!/bin/bash
# Let's first check if there are any test files for wasm_parse.go
fd "wasm_parse_test.go"
# Let's also check for any existing test functions related to parseFundsArg
rg "func.*Test.*parseFundsArg|func.*Test.*ParseFundsArg" --type go
# Let's look at the actual test coverage by searching for any test cases that call parseFundsArg
rg "parseFundsArg\(" --type go
Length of output: 367
Script:
#!/bin/bash
# Let's check the entire file content to understand the test coverage strategy
cat x/evm/precompile/wasm_parse.go
# Also check if there are any integration or end-to-end tests that might cover this functionality
rg -B2 -A2 "parseExecuteArgs|parseQueryArgs" --type go
Length of output: 6133
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-27: x/evm/precompile/wasm_parse.go#L26-L27
Added lines #L26 - L27 were not covered by tests
| reqBz, ok := args[argsIdx].([]byte) | ||
| if !ok { | ||
| err = ErrArgTypeValidation("bytes req", args[argsIdx]) | ||
| return | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for request bytes type assertion error path
Based on the code analysis, there are no existing test cases for the error path in parseQueryArgs when the request bytes type assertion fails. This should be tested to ensure proper error handling.
Example test case to add:
func TestParseQueryArgs_InvalidRequestBytes(t *testing.T) {
p := precompileWasm{}
_, _, err := p.parseQueryArgs([]interface{}{
validContractAddr.Bytes(),
123, // Invalid type: number instead of []byte
})
require.Error(t, err)
require.Contains(t, err.Error(), "bytes req")
}🔗 Analysis chain
Add test coverage for request bytes type assertion.
The type assertion and validation logic is correct, but this error path lacks test coverage.
Let's verify the test coverage:
Would you like me to help generate test cases for this scenario?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any tests for parseQueryArgs invalid request
rg -l "TestParseQueryArgs.*invalid.*request" --type go
Length of output: 56
Script:
#!/bin/bash
# Let's first find the test file for wasm_parse.go
fd "wasm_parse_test.go"
# Then let's look for any existing test cases for parseQueryArgs
rg "func.*Test.*parseQueryArgs" --type go -A 10
# Also check for any test table entries related to invalid arguments
rg "parseQueryArgs.*invalid|invalid.*parseQueryArgs" --type go
Length of output: 138
Script:
#!/bin/bash
# Let's check the entire directory structure to understand where the test might be
fd . --type f --extension go
# Let's also look at the implementation to understand the context better
rg -A 10 "func parseQueryArgs" --type go
Length of output: 16520
Script:
#!/bin/bash
# Let's look for the implementation of parseQueryArgs in wasm_parse.go
rg -A 15 "func parseQueryArgs" x/evm/precompile/wasm_parse.go
# Let's also check for any test files in the same directory
fd . x/evm/precompile/ --type f --extension go
Length of output: 410
Script:
#!/bin/bash
# Let's examine wasm_test.go for any existing test cases related to parseQueryArgs
rg -B 5 -A 10 "parseQueryArgs|TestParseQueryArgs" x/evm/precompile/wasm_test.go
# Also check the implementation context in wasm_parse.go
rg -B 5 -A 10 "parseQueryArgs" x/evm/precompile/wasm_parse.go
Length of output: 415
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 197-199: x/evm/precompile/wasm_parse.go#L197-L199
Added lines #L197 - L199 were not covered by tests
x/evm/precompile/funtoken.go
Outdated
| if readOnly { | ||
| return nil, fmt.Errorf("bankSend cannot be called in read-only mode") |
There was a problem hiding this comment.
Add test coverage for readOnly mode check
The readOnly check is a critical safety measure, but static analysis indicates it lacks test coverage. Please add test cases to verify this behavior.
Would you like me to help generate test cases for the readOnly mode check?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 100-100: x/evm/precompile/funtoken.go#L100
Added line #L100 was not covered by tests
| "exceeded maximum number Nibiru-specific precompiled contract calls in one transaction (%d).", | ||
| maxMultistoreCacheCount, |
There was a problem hiding this comment.
Add test coverage for maximum precompile calls limit.
The error message for exceeding maximum precompile calls lacks test coverage. Additionally, consider improving the error message format.
- Add test coverage for the error case when exceeding
maxMultistoreCacheCount. - Consider this improved error message format:
- "exceeded maximum number Nibiru-specific precompiled contract calls in one transaction (%d).",
+ "exceeded maximum number of Nibiru-specific precompiled contract calls (%d) in one transaction",📝 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.
| "exceeded maximum number Nibiru-specific precompiled contract calls in one transaction (%d).", | |
| maxMultistoreCacheCount, | |
| "exceeded maximum number of Nibiru-specific precompiled contract calls (%d) in one transaction", | |
| maxMultistoreCacheCount, |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 602-603: x/evm/statedb/statedb.go#L602-L603
Added lines #L602 - L603 were not covered by tests
| default: | ||
| // Note that this code path should be impossible to reach since | ||
| // "DecomposeInput" parses methods directly from the ABI. | ||
| err = fmt.Errorf("invalid method called with name \"%s\"", method.Name) | ||
| err = fmt.Errorf("invalid method called with name \"%s\"", startResult.Method.Name) |
There was a problem hiding this comment.
Add unit tests for handling invalid method calls.
The new error handling at line 58 provides a specific error message when an invalid method is called. To ensure robustness, please add unit tests that cover this scenario to verify that the error is correctly triggered and handled.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-58: x/evm/precompile/wasm.go#L58
Added line #L58 was not covered by tests
x/evm/precompile/wasm.go
Outdated
| if readOnly { | ||
| return nil, errors.New("wasm execute cannot be called in read-only mode") |
There was a problem hiding this comment.
Add unit tests for read-only mode restriction in execute method.
The execute method now checks for readOnly mode and returns an error if it's true. Please add unit tests to confirm that this error handling works as expected when the method is invoked in read-only mode.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 127-127: x/evm/precompile/wasm.go#L127
Added line #L127 was not covered by tests
x/evm/precompile/wasm.go
Outdated
| if readOnly { | ||
| return nil, errors.New("wasm instantiate cannot be called in read-only mode") |
There was a problem hiding this comment.
Add unit tests for read-only mode restriction in instantiate method.
The instantiate method includes a check for readOnly mode and returns an error if it's true. Ensure that there are unit tests covering this scenario to validate the correctness of the error handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 211-211: x/evm/precompile/wasm.go#L211
Added line #L211 was not covered by tests
x/evm/precompile/wasm.go
Outdated
| if readOnly { | ||
| return nil, errors.New("wasm executeMulti cannot be called in read-only mode") |
There was a problem hiding this comment.
Add unit tests for read-only mode restriction in executeMulti method.
The executeMulti method now returns an error when called in read-only mode. Please include unit tests to verify that this restriction is properly enforced and that the error message is returned correctly.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 262-262: x/evm/precompile/wasm.go#L262
Added line #L262 was not covered by tests
| func assertNotReadonlyTx(readOnly bool, isTx bool) error { | ||
| if readOnly && isTx { | ||
| return errors.New("cannot write state from staticcall (a read-only call)") |
There was a problem hiding this comment.
You can add the method name as an argument to keep the error messages consistent across each tx method using the same function
There was a problem hiding this comment.
I originally got rid of assertNotReadonlyTx because whenever it was invoked, the isTx argument was always true, which made that parameter redundant because assertNotReadonlyTx in a mutating precompile. I've replaced it and added the gethabi.Method as a parameter to extract the method name into the error string.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (18)
cmd/nibid/cmd/base64.go (1)
Line range hint
16-24: Consider adding input validation for the message data.While the UnmarshalJSON will catch invalid JSON, we could improve user experience by validating the input format upfront.
Here's a suggested improvement:
Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + if len(args[0]) == 0 { + return fmt.Errorf("message data cannot be empty") + } + if args[0][0] != '{' || args[0][len(args[0])-1] != '}' { + return fmt.Errorf("message data must be a JSON object") + } clientCtx := client.GetClientContextFromCmd(cmd)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: cmd/nibid/cmd/base64.go#L36
Added line #L36 was not covered by testsx/devgas/v1/types/params.go (2)
Line range hint
40-60: Consider enhancing error messages with actual values.While the validation logic is thorough, the error messages could be more helpful by including the actual invalid value in the error message.
Consider this improvement:
if v.IsNegative() { - return fmt.Errorf("value cannot be negative: %T", i) + return fmt.Errorf("value cannot be negative: %v", v) } if v.GT(math.LegacyOneDec()) { - return fmt.Errorf("value cannot be greater than 1: %T", i) + return fmt.Errorf("value cannot be greater than 1: %v", v) }
Line range hint
62-74: Optimize type assertion usage.The function performs the same type assertion twice, which is unnecessary and could impact performance.
Consider this optimization:
func validateArray(i any) error { - _, ok := i.([]string) + arr, ok := i.([]string) if !ok { return fmt.Errorf("invalid parameter type: %T", i) } - for _, denom := range i.([]string) { + for _, denom := range arr { if denom == "" { return fmt.Errorf("denom cannot be blank") } } return nil }x/tokenfactory/types/codec.go (1)
61-61: LGTM! Good modernization of type declaration.The change from
interface{}toanyaligns with modern Go practices. Theanytype was introduced in Go 1.18 as a more readable alias forinterface{}, making the code more idiomatic without affecting runtime behavior.Consider applying this modernization consistently across the codebase for better maintainability. The
anytype is preferred in modern Go as it:
- Improves code readability
- Makes the intent clearer
- Follows current Go best practices
x/evm/params.go (1)
79-79: LGTM with a minor suggestion for error message improvementThe update to
anyis good. Consider making the error message more descriptive for better debugging.- return fmt.Errorf("invalid EIP slice type: %T", i) + return fmt.Errorf("expected []int64 for EIPs, got: %T", i)eth/rpc/backend/node_info.go (2)
Line range hint
52-58: Document or remove commented fieldsThe commented-out fields (
highestBlock,pulledStates,knownStates) with "NA" comments could be better documented to explain why they're not applicable in this implementation.Consider either:
- Adding a comment explaining why these Ethereum standard fields are not applicable in Nibiru's context, or
- Removing them entirely if they're not part of the interface contract
return map[string]any{ "startingBlock": hexutil.Uint64(status.SyncInfo.EarliestBlockHeight), "currentBlock": hexutil.Uint64(status.SyncInfo.LatestBlockHeight), - // "highestBlock": nil, // NA - // "pulledStates": nil, // NA - // "knownStates": nil, // NA }, nil🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 52-52: eth/rpc/backend/node_info.go#L52
Added line #L52 was not covered by tests
Test coverage exists but needs enhancement
The
TestSyncingfunction exists but only tests the non-syncing scenario. The current test coverage should be expanded to include:
- The syncing scenario where
status.SyncInfo.CatchingUpis true- Error handling when
clientCtx.Client.StatusfailsExample test cases to add:
// Test when node is syncing s.mockStatusResponse.SyncInfo.CatchingUp = true s.mockStatusResponse.SyncInfo.EarliestBlockHeight = 100 s.mockStatusResponse.SyncInfo.LatestBlockHeight = 200 // Test error handling s.mockClient.StatusFunc = func() error { return errors.New("status error") }🔗 Analysis chain
Line range hint
52-58: Add test coverage for sync status reportingThe sync status reporting code path lacks test coverage. Consider adding test cases for both syncing and non-syncing scenarios.
Would you like me to help generate comprehensive test cases for this functionality?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing tests of the Syncing method rg -l "TestBackend.*Syncing" --type goLength of output: 40
Script:
#!/bin/bash # Let's try a broader search for test files related to the backend fd ".*_test.go" eth/rpc/backend/ # Also search for any Syncing-related tests with a broader pattern rg "func.*Syncing.*Test" --type go # Check the actual implementation to understand the test scope rg "func.*Syncing" --type go -A 5Length of output: 1895
Script:
#!/bin/bash # Let's examine the actual test implementation in node_info_test.go cat eth/rpc/backend/node_info_test.go # Also check the implementation in node_info.go for full context cat eth/rpc/backend/node_info.goLength of output: 4393
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 52-52: eth/rpc/backend/node_info.go#L52
Added line #L52 was not covered by testseth/eip712/message.go (1)
Line range hint
13-18: Consider enhancing struct documentationWhile the code is well-documented overall, consider adding a doc comment for the
eip712MessagePayloadstruct to explain its purpose and field usage.Add documentation like:
+// eip712MessagePayload represents a parsed and flattened EIP-712 message +// containing the original payload, number of messages, and the parsed message map. type eip712MessagePayload struct { payload gjson.Result numPayloadMsgs int message map[string]any }x/inflation/types/params.go (2)
Line range hint
74-82: Consider enhancing polynomial factors validation.While the type change to
anyis good, the validation could be more thorough to ensure the polynomial factors are within reasonable bounds.Consider adding validation for individual factors:
func validatePolynomialFactors(i any) error { v, ok := i.([]sdk.Dec) if !ok { return fmt.Errorf("invalid parameter type: %T", i) } if len(v) == 0 { return errors.New("polynomial factors cannot be empty") } + // Validate each factor + for i, factor := range v { + if factor.IsNil() { + return fmt.Errorf("polynomial factor at index %d is nil", i) + } + } return nil }
Line range hint
142-154: Consider adding an upper bound check.While the validation ensures positive values, consider adding an upper bound check to prevent unrealistic values (e.g., more than 365 periods per year).
func validatePeriodsPerYear(i any) error { val, ok := i.(uint64) if !ok { return fmt.Errorf("invalid parameter type: %T", i) } if val <= 0 { return fmt.Errorf("periods per year must be positive: %d", val) } + if val > 365 { + return fmt.Errorf("periods per year must not exceed 365: %d", val) + } + return nil }x/evm/precompile/precompile.go (1)
Based on the search results, I can now generate the final response since I've found that while there are tests for the precompile package, there's no specific test coverage for the
decomposeInputfunction's error handling paths.Add test coverage for decomposeInput error handling
The function needs test coverage for these critical error paths:
- Input less than 4 bytes
- Invalid method ID parsing
- Input argument unpacking failures
Add test cases in
x/evm/precompile/precompile_test.go:func TestDecomposeInput(t *testing.T) { tests := []struct { name string input []byte wantErr string }{ { name: "input too short", input: []byte{0x1, 0x2, 0x3}, wantErr: "input \"0x010203\" too short to extract method ID", }, // Add more test cases } // Test implementation }🔗 Analysis chain
Line range hint
88-107: LGTM! Good refactoring of decomposeInput.The changes improve the code by:
- Making the function unexported as it's an internal utility
- Using modern Go idiom
[]anyinstead of[]interface{}- Simplifying error message construction
Add test coverage for error handling.
The error handling logic on line 93 needs test coverage to ensure robustness.
Would you like me to help generate test cases for the error handling scenarios?
Let me search for test files in the precompile directory to better understand the test coverage.
Let me check specifically for any test cases related to ABI method handling and input validation in the precompile package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check existing test coverage for decomposeInput error cases rg -A 5 "TestDecomposeInput|Test.*decomposeInput"Length of output: 1693
Script:
#!/bin/bash # Search for test files in precompile directory fd -e go -e _test.go . x/evm/precompile/ # Check for any existing test cases that might be related to input validation rg -A 10 "func Test.*" x/evm/precompile/*_test.goLength of output: 1218
Script:
#!/bin/bash # Search for test cases related to ABI and input validation rg -B 2 -A 10 "abi.*Method|input.*byte|Unpack" x/evm/precompile/*_test.go # Check if there are any mock or test utility functions that might be handling input validation rg -B 2 -A 5 "mock.*input|test.*input|helper.*input" x/evm/precompile/Length of output: 2291
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 93-93: x/evm/precompile/precompile.go#L93
Added line #L93 was not covered by testseth/rpc/backend/tracing.go (1)
Line range hint
256-261: LGTM with refactor suggestionThe variable type change to
anyis consistent with the method signature update. However, there's an opportunity to reduce code duplication.Consider extracting the common unmarshaling logic into a helper function since both
TraceTransactionandTraceCallshare similar result handling code:func unmarshalTraceResult(data []byte) (any, error) { var result any if err := json.Unmarshal(data, &result); err != nil { return nil, err } return result, nil }This would simplify both methods and make future modifications easier to maintain.
eth/rpc/rpcapi/debugapi/api.go (1)
118-118: LGTM! Consider documenting the return type.The change to
anyis good. Consider adding a comment to document the expected concrete types that this method can return to help future maintainers.Example documentation:
// TraceCall ... (existing godoc) // Returns: // - *evm.ExecutionResult: for successful traces // - error: if the trace failseth/rpc/rpcapi/websockets.go (3)
Line range hint
302-315: Consider enhancing error messagesWhile the parameter validation is thorough, the error messages could be more descriptive to help with debugging. Consider including:
- Expected parameter types
- Received parameter types
- Valid parameter examples
- s.sendErrResponse(wsConn, "invalid parameters") + s.sendErrResponse(wsConn, fmt.Sprintf("invalid parameters: expected array, got %T", msg["params"]))🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 302-303: eth/rpc/rpcapi/websockets.go#L302-L303
Added lines #L302 - L303 were not covered by tests
Line range hint
465-561: Refactor complex subscription logicThe log subscription handling contains complex nested type assertions and error handling. Consider:
- Extracting address and topic parsing into separate functions
- Using a dedicated type for subscription parameters
- Adding comprehensive test coverage
Example refactor for topic handling:
type LogSubscriptionParams struct { Addresses []common.Address Topics [][]common.Hash } func parseTopics(topics []any) ([][]common.Hash, error) { result := make([][]common.Hash, len(topics)) for i, topic := range topics { parsed, err := parseTopic(topic) if err != nil { return nil, err } result[i] = parsed } return result, nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 465-465: eth/rpc/rpcapi/websockets.go#L465
Added line #L465 was not covered by tests
[warning] 469-469: eth/rpc/rpcapi/websockets.go#L469
Added line #L469 was not covered by tests
WebSocket operations require test coverage
The WebSocket implementation in
eth/rpc/rpcapi/websockets.golacks test coverage for critical operations including:
- WriteJSON method for concurrent write operations
- ReadMessage method for handling incoming messages
- Connection error handling and cleanup
- Subscription message handling
The codebase search confirms there are no dedicated WebSocket tests, despite the complexity of the WebSocket server implementation that handles:
- JSON-RPC message parsing
- Subscription management
- Concurrent access control
- Error handling and connection cleanup
Please add test coverage focusing on these core WebSocket operations to ensure reliability of the WebSocket server implementation.
🔗 Analysis chain
Line range hint
160-165: Add test coverage for WebSocket operationsWhile the implementation is correct, this critical WebSocket operation lacks test coverage. Consider adding tests for:
- Writing different types of messages
- Handling connection errors
- Concurrent write operations
Let me search for WebSocket related tests more specifically in the eth/rpc directory and check for any test implementations that might be using WebSocket connections.
Let me try searching for tests that might be using WebSocket connections in different ways, including looking for the
wsConntype usage and general WebSocket related test files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing WebSocket tests rg -l "func.*Test.*WebSocket|func.*Test.*WS"Length of output: 16686
Script:
#!/bin/bash # Search for WebSocket related tests in eth/rpc directory rg -l "TestWS|TestWebSocket" eth/rpc/ # Search for WebSocket test implementations ast-grep --pattern 'func Test$_($$$) { $$$ ws$_ $$$ }' # Search for WebSocket connection tests specifically rg -A 5 "func.*Test.*WebSocket|func.*Test.*WS" eth/rpc/Length of output: 160
Script:
#!/bin/bash # Search for any tests using the wsConn type rg -A 5 "wsConn" eth/rpc/ # Search for any WebSocket related test files fd -e go -e _test.go . eth/rpc/ | xargs rg -l "websocket|ws\." # Look for WebSocket connection handling in tests ast-grep --pattern 'type $_ struct { $$$ conn *websocket.$_ $$$ }'Length of output: 12968
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 160-160: eth/rpc/rpcapi/websockets.go#L160
Added line #L160 was not covered by testseth/rpc/rpcapi/eth_api.go (1)
33-34: LGTM! Good modernization of type signatures.The change from
interface{}toanyaligns with modern Go conventions and improves code readability while maintaining type safety.Consider adding a comment in the interface documentation mentioning that
anyis used for compatibility with Ethereum JSON-RPC responses, which can have varying types.Also applies to: 100-103, 108-108
eth/rpc/rpcapi/eth_filters_api.go (1)
Line range hint
624-663: Consider adding comprehensive test cases.The
GetFilterChangesmethod handles multiple filter types and edge cases. Consider adding test cases for:
- PendingTransactionsSubscription
- BlocksSubscription
- LogsSubscription
- MinedAndPendingLogsSubscription
- Invalid filter types
- Non-existent filter IDs
- Timer expiration scenarios
Would you like me to help generate a comprehensive test suite for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 624-624: eth/rpc/rpcapi/eth_filters_api.go#L624
Added line #L624 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (35)
CHANGELOG.md(2 hunks)app/server/config/server_config.go(1 hunks)cmd/nibid/cmd/base64.go(1 hunks)cmd/nibid/cmd/decode_base64.go(4 hunks)eth/eip712/eip712_legacy.go(4 hunks)eth/eip712/encoding_legacy.go(1 hunks)eth/eip712/message.go(2 hunks)eth/rpc/backend/blocks.go(3 hunks)eth/rpc/backend/node_info.go(2 hunks)eth/rpc/backend/tracing.go(4 hunks)eth/rpc/backend/utils.go(1 hunks)eth/rpc/rpc.go(1 hunks)eth/rpc/rpcapi/debugapi/api.go(2 hunks)eth/rpc/rpcapi/eth_api.go(5 hunks)eth/rpc/rpcapi/eth_filters_api.go(1 hunks)eth/rpc/rpcapi/websockets.go(12 hunks)x/common/testutil/nullify.go(1 hunks)x/common/testutil/testnetwork/logger.go(1 hunks)x/devgas/v1/types/params.go(3 hunks)x/epochs/types/identifier.go(1 hunks)x/evm/errors.go(1 hunks)x/evm/keeper/grpc_query.go(2 hunks)x/evm/params.go(2 hunks)x/evm/precompile/errors.go(1 hunks)x/evm/precompile/funtoken.go(2 hunks)x/evm/precompile/oracle.go(4 hunks)x/evm/precompile/precompile.go(5 hunks)x/evm/precompile/test/export.go(5 hunks)x/evm/precompile/wasm.go(8 hunks)x/evm/precompile/wasm_parse.go(5 hunks)x/evm/statedb/statedb.go(2 hunks)x/evm/vmtracer.go(1 hunks)x/inflation/types/params.go(5 hunks)x/oracle/types/hash.go(1 hunks)x/tokenfactory/types/codec.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/server/config/server_config.go
- x/oracle/types/hash.go
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- x/evm/precompile/oracle.go
- x/evm/statedb/statedb.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cmd/nibid/cmd/base64.go
[warning] 36-36: cmd/nibid/cmd/base64.go#L36
Added line #L36 was not covered by tests
cmd/nibid/cmd/decode_base64.go
[warning] 65-65: cmd/nibid/cmd/decode_base64.go#L65
Added line #L65 was not covered by tests
eth/rpc/backend/node_info.go
[warning] 52-52: eth/rpc/backend/node_info.go#L52
Added line #L52 was not covered by tests
eth/rpc/rpc.go
[warning] 119-119: eth/rpc/rpc.go#L119
Added line #L119 was not covered by tests
[warning] 127-127: eth/rpc/rpc.go#L127
Added line #L127 was not covered by tests
eth/rpc/rpcapi/eth_api.go
[warning] 153-153: eth/rpc/rpcapi/eth_api.go#L153
Added line #L153 was not covered by tests
[warning] 362-362: eth/rpc/rpcapi/eth_api.go#L362
Added line #L362 was not covered by tests
[warning] 370-370: eth/rpc/rpcapi/eth_api.go#L370
Added line #L370 was not covered by tests
[warning] 399-399: eth/rpc/rpcapi/eth_api.go#L399
Added line #L399 was not covered by tests
eth/rpc/rpcapi/eth_filters_api.go
[warning] 624-624: eth/rpc/rpcapi/eth_filters_api.go#L624
Added line #L624 was not covered by tests
eth/rpc/rpcapi/websockets.go
[warning] 160-160: eth/rpc/rpcapi/websockets.go#L160
Added line #L160 was not covered by tests
[warning] 206-206: eth/rpc/rpcapi/websockets.go#L206
Added line #L206 was not covered by tests
[warning] 302-303: eth/rpc/rpcapi/websockets.go#L302-L303
Added lines #L302 - L303 were not covered by tests
[warning] 339-339: eth/rpc/rpcapi/websockets.go#L339
Added line #L339 was not covered by tests
[warning] 365-365: eth/rpc/rpcapi/websockets.go#L365
Added line #L365 was not covered by tests
[warning] 465-465: eth/rpc/rpcapi/websockets.go#L465
Added line #L465 was not covered by tests
[warning] 469-469: eth/rpc/rpcapi/websockets.go#L469
Added line #L469 was not covered by tests
[warning] 478-478: eth/rpc/rpcapi/websockets.go#L478
Added line #L478 was not covered by tests
[warning] 505-505: eth/rpc/rpcapi/websockets.go#L505
Added line #L505 was not covered by tests
[warning] 514-514: eth/rpc/rpcapi/websockets.go#L514
Added line #L514 was not covered by tests
[warning] 541-541: eth/rpc/rpcapi/websockets.go#L541
Added line #L541 was not covered by tests
x/evm/errors.go
[warning] 109-109: x/evm/errors.go#L109
Added line #L109 was not covered by tests
x/evm/precompile/errors.go
[warning] 13-14: x/evm/precompile/errors.go#L13-L14
Added lines #L13 - L14 were not covered by tests
x/evm/precompile/funtoken.go
[warning] 105-105: x/evm/precompile/funtoken.go#L105
Added line #L105 was not covered by tests
x/evm/precompile/precompile.go
[warning] 93-93: x/evm/precompile/precompile.go#L93
Added line #L93 was not covered by tests
x/evm/precompile/wasm.go
[warning] 59-59: x/evm/precompile/wasm.go#L59
Added line #L59 was not covered by tests
[warning] 132-132: x/evm/precompile/wasm.go#L132
Added line #L132 was not covered by tests
[warning] 216-216: x/evm/precompile/wasm.go#L216
Added line #L216 was not covered by tests
[warning] 267-267: x/evm/precompile/wasm.go#L267
Added line #L267 was not covered by tests
x/evm/precompile/wasm_parse.go
[warning] 26-27: x/evm/precompile/wasm_parse.go#L26-L27
Added lines #L26 - L27 were not covered by tests
🔇 Additional comments (61)
x/common/testutil/testnetwork/logger.go (1)
14-15: LGTM! Good modernization of type declarations.
The change from interface{} to any aligns with modern Go conventions introduced in Go 1.18+. This improves code readability while maintaining full compatibility since any is just an alias for interface{}.
x/epochs/types/identifier.go (1)
23-23: LGTM! Good modernization of type declaration.
The change from interface{} to any aligns with modern Go practices while maintaining the same functionality. This type alias was introduced in Go 1.18 to improve code readability.
Let's verify this is part of a consistent cleanup effort across the codebase:
✅ Verification successful
Based on the search results, I can generate the final response now:
LGTM! The change from interface{} to any is part of a broader modernization effort.
The codebase shows consistent adoption of any type across multiple packages, while remaining interface{} usages are primarily in generated protobuf files (.pb.go). This change aligns with modern Go practices and is part of a systematic cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for remaining interface{} usage that might need similar updates
# Expect: This change is part of a broader cleanup effort
# Search for remaining interface{} declarations
rg 'interface\{\}' --type go
# Search for any type usage to confirm modernization pattern
rg '\b(any)\b' --type go
Length of output: 40637
cmd/nibid/cmd/base64.go (1)
Line range hint 36-41: LGTM! Consider adding tests for this command.
The change from interface{} to any aligns with modern Go practices. However, this code path lacks test coverage.
Let's verify the test coverage:
Would you like me to help create test cases for this command? The tests should cover:
- Successful message building
- Invalid JSON input handling
- Malformed message handling
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: cmd/nibid/cmd/base64.go#L36
Added line #L36 was not covered by tests
x/common/testutil/nullify.go (1)
19-19: LGTM! Good modernization of Go type usage.
The change from interface{} to any aligns with modern Go practices and improves code readability while maintaining the same functionality. This is part of a broader initiative to update type declarations across the codebase, making it more idiomatic.
x/devgas/v1/types/params.go (2)
Line range hint 31-38: LGTM! Good use of the any type.
The change from interface{} to any is a positive modernization that aligns with current Go idioms while maintaining the same functionality.
Line range hint 31-74: Verify consistent type usage across the codebase.
Let's ensure these parameter validation patterns are consistently applied across other modules.
✅ Verification successful
Parameter validation patterns are consistently implemented across modules
The validation functions in x/devgas/v1/types/params.go follow the same pattern used throughout the codebase:
- All validation functions use
anytype for parameters - Type assertions follow a consistent pattern with
x.(Type)and error handling - Error messages follow the same format:
"invalid parameter type: %T"
This is evidenced by identical implementations in:
x/inflation/types/params.gox/evm/params.go- Other modules using the same validation pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other validation functions that might need similar updates
rg -t go "func validate.*\(.*interface{}\)" -g '!vendor/*'
# Look for similar parameter validation patterns
ast-grep --pattern 'func validate$_($_) error {
$_, ok := $_.(type)
if !ok {
return fmt.Errorf($_)
}
}'
Length of output: 320
Script:
#!/bin/bash
# Search for other validation functions that might need similar updates
rg -t go "func validate.*\(.*any\)" -g '!vendor/*'
# Look for similar parameter validation patterns with type assertions
ast-grep --pattern 'func validate$_($_ any) error {
$_, ok := $_.$_
if !ok {
return fmt.Errorf($_)
}
}'
# Search for other parameter validation functions
rg -t go "func validate.*\(i any\)" -A 5 -g '!vendor/*'
Length of output: 4107
x/evm/precompile/errors.go (1)
11-16: LGTM! Improved error handling with better context.
The refactoring improves the function by:
- Removing the redundant
isTxparameter - Adding method context to error messages
- Providing clearer guidance about read-only context violations
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-14: x/evm/precompile/errors.go#L13-L14
Added lines #L13 - L14 were not covered by tests
x/evm/params.go (1)
38-38: LGTM: Good modernization of type declaration
The update from interface{} to any aligns with modern Go practices while maintaining the same functionality. This change improves code readability without affecting the behavior.
eth/rpc/backend/node_info.go (1)
42-42: LGTM: Good modernization of type syntax
The change from interface{} to any aligns with modern Go practices (Go 1.18+) and improves code readability while maintaining the same functionality.
x/evm/vmtracer.go (1)
54-55: LGTM! Good modernization of type declaration.
The change from interface{} to any aligns with modern Go practices while maintaining identical functionality. This type alias was introduced in Go 1.18 to improve code readability.
x/evm/errors.go (1)
109-111: LGTM: Good modernization of type signature
The change from interface{} to any aligns with modern Go practices and improves code readability while maintaining the same functionality.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 109-109: x/evm/errors.go#L109
Added line #L109 was not covered by tests
eth/eip712/message.go (2)
17-17: LGTM: Good modernization of type usage
The change from interface{} to any aligns with modern Go practices and improves code readability while maintaining identical behavior.
37-37: LGTM: Consistent type assertion update
The type assertion change maintains consistency with the struct field update while preserving proper error handling and type safety.
x/inflation/types/params.go (3)
Line range hint 86-111: LGTM! Comprehensive validation logic.
The type change to any is appropriate, and the validation logic thoroughly checks distribution ratios, including negative values and total distribution equality.
Line range hint 112-120: LGTM! Simple and effective boolean validation.
Line range hint 129-141: LGTM! Proper validation of epochs per period.
x/evm/precompile/wasm_parse.go (4)
25-50: LGTM! Improved funds parsing with defensive programming.
The implementation is well-structured with:
- Early nil check returning empty coins
- Clear type assertions with descriptive error messages
- Safe coin construction avoiding panic-prone sdk.NewCoin
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-27: x/evm/precompile/wasm_parse.go#L26-L27
Added lines #L26 - L27 were not covered by tests
Line range hint 138-153: LGTM! Enhanced readability and error handling.
Good improvements with:
- Clear section comments for better code navigation
- Detailed error messages that include context
166-166: LGTM! Consistent error handling.
Good addition of error context wrapping for funds parsing errors.
195-199: LGTM! Improved request validation.
Good addition of explicit type assertion and error handling for request bytes.
cmd/nibid/cmd/decode_base64.go (4)
30-30: LGTM: Good modernization of type declaration
The change from interface{} to any aligns with modern Go best practices while maintaining the same functionality.
Line range hint 53-64: LGTM: Consistent type modernization with proper field handling
The update to map[string]any maintains type safety while modernizing the code. The handling of TypeURL and Value fields remains robust.
96-96: LGTM: Consistent type modernization
The change to any type is consistent with the module's modernization while maintaining robust error handling and validation.
65-68: Add test coverage for array handling in parseStargateMsgs
While the type change to []any is good, this section lacks test coverage. Consider adding test cases that exercise the array handling logic to ensure proper recursive parsing of nested arrays.
Let's verify the current test coverage:
Would you like me to help create test cases for array handling scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 65-65: cmd/nibid/cmd/decode_base64.go#L65
Added line #L65 was not covered by tests
x/evm/precompile/precompile.go (4)
Line range hint 182-211: LGTM! Improved state synchronization.
The changes enhance state consistency by:
- Properly handling precompile state changes through CacheCtx
- Adding explicit state synchronization with StateDB.CommitCacheCtx()
- Improving error handling for state operations
Line range hint 214-224: LGTM! Clear and descriptive map name.
The rename from precompileMethodIsTxMap to isMutation better reflects the map's purpose of identifying state-mutating operations.
Line range hint 112-134: Verify gas calculation changes against EVM specifications.
The gas calculation logic has been simplified and now uses a flat cost plus per-byte cost based on whether the operation is a mutation. While the implementation looks correct, please ensure:
- The gas costs align with EVM specifications for precompiled contracts
- The gas calculation remains consistent with the previous implementation for all methods
#!/bin/bash
# Search for gas-related tests and calculations
echo "Searching for gas calculation tests..."
rg -l "requiredGas|RequiredGas" --type go
echo "Searching for gas cost constants..."
rg "gasCfg|GasConfig|costPerByte|costFlat" --type go136-145:
Maintain consistency in type usage.
The Args field in OnRunStartResult uses []any while earlier changes moved from []interface{} to []any. Consider using []any consistently throughout the codebase.
Apply this diff to maintain consistency:
- Args []any
+ Args []interface{}Likely invalid or redundant comment.
x/evm/precompile/funtoken.go (2)
36-40: LGTM! Improved ABI handling.
The changes improve consistency by:
- Using a common gas calculation implementation
- Centralizing the ABI definition in the embeds package
This addresses the previous concern about ABI consistency.
103-103: LGTM! Improved state management.
Using CacheCtx instead of Ctx is a good practice as it ensures state changes can be reverted if the transaction fails.
eth/eip712/encoding_legacy.go (1)
20-21: LGTM! Good modernization of type usage.
The change from interface{} to any aligns with modern Go practices introduced in Go 1.18+. This improves code readability while maintaining the same functionality, as any is simply a type alias for interface{}.
eth/rpc/backend/tracing.go (3)
21-21: LGTM: Good modernization of return type
The change from interface{} to any aligns with modern Go best practices while maintaining the same functionality.
127-127: LGTM: Consistent type update
The variable type change to any is consistent with the method signature update and maintains the same unmarshaling behavior.
222-222: LGTM: Consistent method signature update
The return type change to any maintains consistency with other tracing methods while improving code readability.
eth/rpc/backend/utils.go (1)
112-112: LGTM: Type change aligns with modern Go practices.
The change from interface{} to any improves code readability while maintaining the same functionality.
eth/rpc/rpc.go (1)
117-117: LGTM! Type changes align with Go best practices.
The migration from interface{} to any follows Go 1.18+ recommendations, improving code readability while maintaining functionality.
Let's verify test coverage for the FormatBlock function:
Consider adding test cases to cover the type handling in FormatBlock, particularly:
- Passing different types of transactions
- Verifying map key-value types in the result
- Edge cases with nil or empty values
Would you like me to help generate comprehensive test cases for these scenarios?
Also applies to: 119-119, 127-127
x/evm/precompile/test/export.go (5)
14-14: LGTM: Import additions are appropriate.
The new imports for vm and serverconfig are correctly added and used within the codebase.
Also applies to: 18-18
31-41: LGTM: Improved naming conventions.
The struct renaming from otherArgs to instantiateArgs and field update from InstMsg to InstantiateMsg enhance code clarity by being more descriptive and following Go naming conventions.
59-59: Use any instead of interface{}.
Following modern Go conventions and previous review comments, replace interface{} with any for better readability and consistency.
-callArgs := []any{m.Admin, m.CodeID, msgArgsBz, m.Label, []precompile.WasmBankCoin{}}
+callArgs := []interface{}{m.Admin, m.CodeID, msgArgsBz, m.Label, []precompile.WasmBankCoin{}}113-117: LGTM: Improved error message for path-related issues.
The enhanced error message now includes both paths, making it easier to debug file-related issues.
134-134: LGTM: Enhanced struct with binPath field.
The addition of the binPath field improves traceability by maintaining a reference to the Wasm contract's source location.
x/evm/precompile/wasm.go (7)
45-55: LGTM! Switch case refactoring improves code clarity.
The switch case structure has been updated to use startResult.Method.Name consistently, making the code more maintainable.
78-82: LGTM! Clean refactoring of gas calculation.
The gas calculation has been moved to a helper function while maintaining the same functionality.
163-163: LGTM! Consistent variable naming in query method.
The query method follows the same refactoring pattern, improving code consistency.
Also applies to: 172-172
329-329: LGTM! Consistent variable naming in queryRaw method.
The queryRaw method follows the same refactoring pattern, improving code consistency.
260-260: Add test coverage for read-only transaction handling in executeMulti method.
The read-only transaction check needs test coverage to ensure proper error handling.
Let's verify the test coverage:
#!/bin/bash
# Search for tests covering read-only transaction scenarios in executeMulti method
rg -l "executeMulti.*readOnly.*true" --type go --glob "*_test.go"Also applies to: 266-267
125-125: Add test coverage for read-only transaction handling in execute method.
The read-only transaction check needs test coverage to ensure proper error handling.
Let's verify the test coverage:
#!/bin/bash
# Search for tests covering read-only transaction scenarios in execute method
rg -l "execute.*readOnly.*true" --type go --glob "*_test.go"Also applies to: 131-132
209-209: Add test coverage for read-only transaction handling in instantiate method.
The read-only transaction check needs test coverage to ensure proper error handling.
Let's verify the test coverage:
#!/bin/bash
# Search for tests covering read-only transaction scenarios in instantiate method
rg -l "instantiate.*readOnly.*true" --type go --glob "*_test.go"Also applies to: 215-216
eth/eip712/eip712_legacy.go (4)
41-41: LGTM: Type declaration modernization
The change from interface{} to any follows modern Go conventions and improves code readability.
61-61: LGTM: Consistent type assertion update
The type assertion change maintains consistency with modern Go type declarations while preserving proper error handling.
142-142: LGTM: Parameter type modernization
The parameter type update to any aligns with modern Go practices while maintaining the existing reflection functionality.
164-165: LGTM: Struct field type modernization
The Value field type update in CosmosAnyWrapper maintains consistency with modern Go type declarations while preserving the protobuf Any type unpacking functionality.
eth/rpc/rpcapi/debugapi/api.go (1)
68-68: LGTM! Good modernization of Go code.
The change from interface{} to any aligns with modern Go conventions introduced in Go 1.18+.
eth/rpc/backend/blocks.go (3)
60-60: LGTM: Type update aligns with modern Go conventions
The change from interface{} to any is a safe improvement that follows modern Go conventions while maintaining identical functionality.
88-88: LGTM: Consistent type update
The change maintains consistency with the previous method's type update.
351-352: LGTM: Type updates complete the consistent modernization
The changes to both the return type and local variable type complete the modernization of type declarations in this file, maintaining consistency throughout the block processing logic.
eth/rpc/rpcapi/websockets.go (2)
44-46: LGTM: Type system modernization
The change from interface{} to any aligns with Go 1.18+ best practices, improving code readability while maintaining the same functionality.
Also applies to: 56-57
Line range hint 160-561: Critical: Improve test coverage
Several critical components lack test coverage:
- WebSocket message handling
- Subscription parameter validation
- Log filtering and topic parsing
- Error scenarios
Consider adding the following test cases:
- Invalid parameter formats
- Malformed WebSocket messages
- Concurrent subscription handling
- Connection error scenarios
#!/bin/bash
# Check current test coverage
echo "Searching for existing test files..."
fd -e go -e _test.go
echo "Checking for mock implementations..."
rg "type.*Mock.*interface|type.*mock.*interface"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 465-465: eth/rpc/rpcapi/websockets.go#L465
Added line #L465 was not covered by tests
[warning] 469-469: eth/rpc/rpcapi/websockets.go#L469
Added line #L469 was not covered by tests
eth/rpc/rpcapi/eth_api.go (1)
33-34: Verify backward compatibility with existing clients.
While the type changes from interface{} to any are internally compatible, please verify that this doesn't affect JSON serialization/deserialization for existing clients.
Also applies to: 100-103, 108-108, 147-147, 153-153, 362-362, 370-370, 399-399
✅ Verification successful
Type changes from interface{} to any are safe and backward compatible
The verification shows that:
- The codebase has already migrated most RPC-related code to use
anytype - Remaining
interface{}usage is primarily in test files - The change is purely syntactic as
anyis just an alias forinterface{}introduced in Go 1.18
This change:
- Maintains JSON serialization compatibility since
anyandinterface{}are identical at runtime - Follows modern Go conventions without breaking API contracts
- Does not affect client implementations as the wire format remains unchanged
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RPC client implementations that might be affected by the type changes
rg -g '!vendor/*' -g '*.go' 'json\.((Un)?[Mm]arshal|Decoder|Encoder).*interface\{\}'
Length of output: 86
Script:
#!/bin/bash
# Let's verify the type changes and their impact more comprehensively
# First, check for any interface{} usage in RPC-related files
rg -g '!vendor/*' -g '*.go' 'interface\{\}' ./eth/rpc/
# Then check for any 'any' type usage to see the migration pattern
rg -g '!vendor/*' -g '*.go' '\bany\b' ./eth/rpc/
# Look for JSON marshaling/unmarshaling in RPC handlers
ast-grep --pattern 'func ($x *$_) $_ ($_ $_) (map[string]$_, error) {
$$$
}'
Length of output: 4978
eth/rpc/rpcapi/eth_filters_api.go (1)
624-624: Add test coverage for the updated GetFilterChanges method.
While changing the return type from interface{} to any is a good modernization step, the modified line is not covered by tests. This method handles critical Ethereum RPC functionality and should be thoroughly tested.
Let's verify the existing test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 624-624: eth/rpc/rpcapi/eth_filters_api.go#L624
Added line #L624 was not covered by tests
x/evm/keeper/grpc_query.go (1)
808-808: Usage of 'any' Type May Affect Compatibility
The variable result is declared with type any on line 808. As previously noted, any was introduced in Go 1.18 as an alias for interface{}. To avoid potential compatibility problems with earlier Go versions, verify that the project's Go version requirements are updated, or replace any with interface{}.
| func assertNotReadonlyTx(readOnly bool, method *gethabi.Method) error { | ||
| if readOnly { | ||
| return fmt.Errorf("method %s cannot be called in a read-only context (e.g. staticcall)", method.Name) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add test coverage for the error handling.
The new implementation lacks test coverage. Consider adding test cases to verify:
- Error message format
- Behavior in read-only context
- Behavior in non-read-only context
Would you like me to help generate comprehensive test cases for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-14: x/evm/precompile/errors.go#L13-L14
Added lines #L13 - L14 were not covered by tests
| func (e *RevertError) ErrorData() any { | ||
| return e.reason | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage needed for RevertError.ErrorData() method
Looking at the codebase:
- There are multiple test files in the x/evm directory
- The RevertError type and its methods are defined in errors.go
- No existing tests were found for RevertError.ErrorData() method
Please add test cases to verify:
- The method correctly returns the hex-encoded reason stored in the RevertError struct
- The return type matches the interface requirement (any)
- Integration with NewExecErrorWithReason constructor
Suggested location: x/evm/errors_test.go
🔗 Analysis chain
Add test coverage for ErrorData method
The ErrorData method currently lacks test coverage. Since this is part of the error handling interface, it's important to ensure it behaves correctly under various scenarios.
Let's verify the current test coverage:
Would you like me to help generate test cases for this method? A comprehensive test suite should verify:
- The correct return of the hex-encoded reason
- Proper handling of empty reasons
- Integration with the broader error handling system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing tests of RevertError and ErrorData
rg -l "TestRevertError|ErrorData.*testing"
# Check if there are any test files in the evm package
fd -e go -e _test.go$ x/evm/
Length of output: 407
Script:
#!/bin/bash
# Search for test files in evm directory using correct fd syntax
fd --full-path '.*_test\.go$' 'x/evm'
# Search for RevertError tests using ripgrep with context
rg -C 5 'TestRevertError|func.*test.*RevertError|ErrorData.*test'
# Look for any existing RevertError usage in tests
rg -C 3 'RevertError'
Length of output: 2482
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 109-109: x/evm/errors.go#L109
Added line #L109 was not covered by tests
| func validateUint64(i any) error { | ||
| _, ok := i.(uint64) | ||
| if !ok { | ||
| return fmt.Errorf("invalid genesis state type: %T", i) | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix inconsistent error message.
The error message mentions "genesis state" but this is a parameter validation function.
func validateUint64(i any) error {
_, ok := i.(uint64)
if !ok {
- return fmt.Errorf("invalid genesis state type: %T", i)
+ return fmt.Errorf("invalid parameter type: %T", i)
}
return nil
}📝 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.
| func validateUint64(i any) error { | |
| _, ok := i.(uint64) | |
| if !ok { | |
| return fmt.Errorf("invalid genesis state type: %T", i) | |
| } | |
| return nil | |
| } | |
| func validateUint64(i any) error { | |
| _, ok := i.(uint64) | |
| if !ok { | |
| return fmt.Errorf("invalid parameter type: %T", i) | |
| } | |
| return nil | |
| } |
| @@ -109,7 +109,7 @@ func (b *Backend) getAccountNonce(accAddr common.Address, pending bool, height i | |||
| // See eth_feeHistory method for more details of the return format. | |||
| func (b *Backend) retrieveEVMTxFeesFromBlock( | |||
| tendermintBlock *tmrpctypes.ResultBlock, | |||
| ethBlock *map[string]interface{}, | |||
| ethBlock *map[string]any, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the parameter type.
Since a map is already a reference type, consider simplifying the parameter type from *map[string]any to map[string]any to avoid unnecessary pointer indirection.
-func (b *Backend) retrieveEVMTxFeesFromBlock(
- tendermintBlock *tmrpctypes.ResultBlock,
- ethBlock *map[string]any,
- rewardPercentiles []float64,
- tendermintBlockResult *tmrpctypes.ResultBlockResults,
- targetOneFeeHistory *rpc.OneFeeHistory,
-) error {
+func (b *Backend) retrieveEVMTxFeesFromBlock(
+ tendermintBlock *tmrpctypes.ResultBlock,
+ ethBlock map[string]any,
+ rewardPercentiles []float64,
+ tendermintBlockResult *tmrpctypes.ResultBlockResults,
+ targetOneFeeHistory *rpc.OneFeeHistory,
+) error {This change would require updating the function body to remove the pointer dereference when accessing the map:
- gasLimitUint64, ok := (*ethBlock)["gasLimit"].(hexutil.Uint64)
+ gasLimitUint64, ok := ethBlock["gasLimit"].(hexutil.Uint64)Committable suggestion was skipped due to low confidence.
| // "DecomposeInput" parses methods directly from the ABI. | ||
| err = fmt.Errorf("invalid method called with name \"%s\"", method.Name) | ||
| err = fmt.Errorf("invalid method called with name \"%s\"", startResult.Method.Name) |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage for invalid method handling is needed
The test file shows comprehensive testing for various error scenarios in TestSadArgsCount, including an "invalid method name" test case, but it specifically tests the ABI method packing stage. However, there's no direct test coverage for the runtime error message "invalid method called with name %s" that occurs during method execution.
- Add test case in
x/evm/precompile/wasm_test.goto verify the specific error message when an invalid method is called during execution.
🔗 Analysis chain
Add test coverage for invalid method handling.
The error message for invalid methods needs test coverage to ensure proper error handling.
Let's verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering invalid method scenarios
rg -l "invalid method.*called with name" --type go --glob "*_test.go"
Length of output: 67
Script:
#!/bin/bash
# Let's try a broader search for test files related to WASM precompile
fd "wasm.*_test\.go$"
# Also search for any test cases that might be testing method validation
rg -A 5 "TestWASM.*Method" --type go
Length of output: 127
Script:
#!/bin/bash
# Let's examine the content of the WASM test files to check for invalid method testing
cat x/evm/precompile/wasm_test.go
Length of output: 12816
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 59-59: x/evm/precompile/wasm.go#L59
Added line #L59 was not covered by tests
| @@ -144,13 +144,13 @@ | |||
| } | |||
|
|
|||
| // GetBlockByNumber returns the block identified by number. | |||
| func (e *EthAPI) GetBlockByNumber(ethBlockNum rpc.BlockNumber, fullTx bool) (map[string]interface{}, error) { | |||
| func (e *EthAPI) GetBlockByNumber(ethBlockNum rpc.BlockNumber, fullTx bool) (map[string]any, error) { | |||
There was a problem hiding this comment.
Add test coverage for modified RPC methods.
The following methods lack test coverage:
GetBlockByNumberGetBlockByHashGetUncleByBlockHashAndIndexGetUncleByBlockNumberAndIndexSyncing
These are critical RPC methods that should be thoroughly tested to ensure compatibility with Ethereum clients.
Would you like me to help generate comprehensive test cases for these methods? The test suite should include:
- Happy path scenarios with various block numbers/hashes
- Edge cases (e.g., pending blocks, latest blocks)
- Error cases (e.g., invalid block numbers, non-existent blocks)
Also applies to: 153-153, 362-362, 370-370, 399-399
| @@ -736,7 +736,7 @@ func (k *Keeper) TraceEthTxMsg( | |||
| traceConfig *evm.TraceConfig, | |||
| commitMessage bool, | |||
| tracerJSONConfig json.RawMessage, | |||
| ) (*interface{}, uint, error) { | |||
| ) (*any, uint, error) { | |||
There was a problem hiding this comment.
Potential Compatibility Issue with 'any' Type
The return type of the TraceEthTxMsg function has been changed from *interface{} to *any. The any type is an alias for interface{} introduced in Go 1.18. If the project aims to maintain compatibility with versions of Go older than 1.18, this change could cause build issues. Please ensure that the minimum Go version requirement is updated accordingly or consider reverting to interface{} for broader compatibility.
General code cleanups on Precompile and StateDB journal sections.
Summary by CodeRabbit
Release Notes
New Features
JournalChangestruct for state consistency during transactions.Fixes and Improvements
Testing Enhancements
Refactor
anyfor better clarity and consistency.