feat(evm): gas usage in precompiles: limits, local gas meters #2093
Conversation
WalkthroughThe changes in this pull request include updates to the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 #2093 +/- ##
==========================================
+ Coverage 64.68% 64.75% +0.06%
==========================================
Files 272 273 +1
Lines 21394 21414 +20
==========================================
+ Hits 13839 13866 +27
+ Misses 6594 6589 -5
+ Partials 961 959 -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: 23
🧹 Outside diff range and nitpick comments (20)
x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol (2)
7-7: Consider adding explicit visibility modifier.The
erc20state variable should have an explicit visibility modifier for better code clarity.- address erc20; + address private immutable erc20;
6-47: Consider architectural improvements.Suggestions for better architecture:
- Define an interface for the FunToken precompile
- Add events for successful transfers
- Consider implementing a more robust error handling mechanism
Example interface:
interface IFunTokenPrecompile { function bankSend( address token, uint256 amount, string memory recipient ) external returns (bool); }Example event:
event BankSendExecuted( address indexed token, uint256 amount, string recipient, uint256 gasUsed );x/evm/evmtest/test_deps.go (1)
64-67: Add documentation for the ResetGasMeter method.The implementation looks good, but consider adding a doc comment to explain the method's purpose and when it should be used in tests.
+// ResetGasMeter resets both the transient gas state and the gas meter in the EVM context. +// This is useful in tests where you need to clear gas usage between operations. func (deps TestDeps) ResetGasMeter() {🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 64-66: x/evm/evmtest/test_deps.go#L64-L66
Added lines #L64 - L66 were not covered by testsx/evm/precompile/oracle_test.go (1)
15-15: Document the rationale for the gas limit value.Consider adding a comment explaining why 100,000 was chosen as the gas limit for oracle queries. This helps maintainers understand if this value needs adjustment in the future.
x/evm/precompile/oracle.go (1)
Line range hint
41-63: Well-structured gas management implementation.The implementation of local gas meters in the oracle precompile follows good practices:
- Clear separation of gas meter initialization and cleanup
- Proper error propagation
- Safe gas consumption checks
Consider documenting these gas management patterns in a shared location as they might be useful for other precompiles.
x/evm/embeds/embeds.go (1)
81-86: Consider enhancing the documentation with more specific details.While the implementation is correct and follows the package patterns, the documentation could be more specific about:
- The purpose of the local gas limit feature
- How it differs from regular precompile calls
- Expected usage scenarios
Example enhancement:
// SmartContract_TestFunTokenPrecompileLocalGas is a test contract -// which allows precompile execution with custom local gas set (calling precompile within contract) +// which demonstrates and tests the execution of precompiles with custom gas limits +// from within another contract. This enables controlled gas consumption for +// precompile calls, ensuring they operate within a subset of the parent +// contract's gas allowance.x/evm/keeper/erc20.go (1)
16-26: LGTM! Consider enhancing constant documentation.The gas limits are well-structured and appropriate for their respective operations. The deployment limit provides adequate headroom, query limit is suitable for read operations, and execute limit aligns with typical ERC20 operation costs.
Consider adding references to standard ERC20 operation gas costs in the comments to justify these limits. For example:
// Erc20GasLimitExecute used for transfer, mint and burn. -// All must not exceed 200_000 +// Standard ERC20 operations typically consume: +// - transfer: ~65,000 gas +// - mint: ~90,000 gas +// - burn: ~80,000 gas +// 200,000 limit provides sufficient headroom Erc20GasLimitExecute uint64 = 200_000x/evm/precompile/funtoken_test.go (2)
Line range hint
93-153: Consider adding more assertions for gas consumption.The test structure looks good and properly validates the basic functionality. However, since this PR focuses on gas management, consider adding assertions to verify:
- Gas consumption after each operation
- Remaining gas in the gas meter
- Gas refunds if applicable
Example assertion:
// Assert gas consumption is within expected range gasUsed := deps.Ctx.GasMeter().GasConsumed() s.Assert().Greater(gasUsed, uint64(0)) s.Assert().Less(gasUsed, keeper.Erc20GasLimitQuery)
155-218: Enhance gas limit test coverage.While the test effectively validates the happy path for local gas usage, consider adding:
Negative test cases:
- Insufficient gas limit
- Zero gas limit
- Gas limit exceeding parent contract's limit
Specific gas consumption assertions:
- Verify exact gas used by precompile
- Check remaining gas after operation
- Validate gas refunds
Example test cases:
s.T().Log("Fail: insufficient local gas") _, err = deps.EvmKeeper.CallContract( deps.Ctx, embeds.SmartContract_TestFunTokenPrecompileLocalGas.ABI, deps.Sender.EthAddr, &contractAddr, true, precompile.FunTokenGasLimitBankSend, "callBankSendLocalGas", big.NewInt(1), randomAcc.String(), big.NewInt(100), // insufficient gas ) s.Require().ErrorContains(err, "out of gas")x/evm/precompile/wasm_test.go (1)
21-25: LGTM! Consider enhancing the constant documentation.The gas limit constants are well-defined and follow good practices. The values are reasonable for test scenarios and the formatting improves readability.
Consider adding a brief comment explaining the rationale behind these specific gas values to help other developers understand the test constraints:
// rough gas limits for wasm execution - used in tests only +// WasmGasLimitQuery: 200k gas sufficient for read-only operations +// WasmGasLimitExecute: 1M gas sufficient for state-modifying operations const ( WasmGasLimitQuery uint64 = 200_000 WasmGasLimitExecute uint64 = 1_000_000 )x/evm/precompile/test/export.go (1)
24-28: LGTM! Consider enhancing the documentation.The gas limit constants are well-defined and appropriately scoped for test usage.
Consider adding a brief comment explaining the rationale behind these specific gas values to help other developers understand the chosen limits.
x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json (2)
1-63: Consider adding natspec documentation.While the contract implementation looks solid, consider adding natspec documentation in the source contract to:
- Document the gas management behavior
- Explain the relationship with the bankSend precompile
- Provide usage examples with gas limits
1-63: Security considerations for gas management.The implementation allows for custom gas limits but should be used carefully:
- Ensure the parent contract has sufficient gas
- Consider implementing minimum gas requirements
- Add gas estimation helpers
Would you like me to provide implementation examples for these security enhancements?
x/evm/keeper/funtoken_from_erc20_test.go (1)
Line range hint
332-337: Consider enhancing gas consumption verificationWhile the error handling for gas-related issues is comprehensive, consider enhancing the test coverage by:
- Capturing and verifying the exact gas consumption before the error occurs
- Adding assertions to verify that the gas consumption is within expected ranges
- Testing edge cases around the gas limits
This would provide more precise validation of the gas management implementation.
Example enhancement for the malicious transfer test:
s.T().Log("send erc20 tokens to cosmos") + initialGas := deps.Ctx.GasMeter().GasConsumed() _, err = deps.EvmKeeper.CallContract( deps.Ctx, embeds.SmartContract_FunToken.ABI, deps.Sender.EthAddr, &precompile.PrecompileAddr_FunToken, true, precompile.FunTokenGasLimitBankSend, "bankSend", deployResp.ContractAddr, big.NewInt(1), randomAcc.String(), ) + gasUsed := deps.Ctx.GasMeter().GasConsumed() - initialGas + s.Require().Greater(gasUsed, precompile.FunTokenGasLimitBankSend, "gas consumption should exceed limit") s.Require().ErrorContains(err, "gas required exceeds allowance")Also applies to: 384-386
x/evm/keeper/grpc_query.go (1)
803-803: LGTM: Consider adding documentation.The
falseparameter is correctly added for transaction message tracing. Consider adding a comment explaining the purpose of this parameter for better maintainability.- res, _, err := k.ApplyEvmMsg(ctx, msg, tracer, commitMessage, cfg, txConfig, false) + // Pass false to prevent committing state changes during tracing + res, _, err := k.ApplyEvmMsg(ctx, msg, tracer, commitMessage, cfg, txConfig, false)CHANGELOG.md (1)
72-72: Consider expanding the changelog entry to better reflect the changes.The current entry for PR #2093 could be expanded to better document the significant changes introduced, such as:
- Addition of local gas meters for precompile execution
- Support for custom gas limits in precompile calls
- Relocation of CallContractWithInput method
- Addition of gasLimit parameter
- Reorganization of funtoken methods
-- [#2093](https://github.com/NibiruChain/nibiru/pull/2093) - feat(evm): gas usage in precompiles: limits, local gas meters +- [#2093](https://github.com/NibiruChain/nibiru/pull/2093) - feat(evm): gas usage in precompiles: limits, local gas meters. Introduces local gas meters for precompile execution, allowing custom gas limits for precompile calls. Relocates CallContractWithInput method from ERC20 module to msg_server.go, adds gasLimit parameter, and reorganizes funtoken methods.x/evm/keeper/funtoken_from_coin.go (1)
128-133: Ensure Atomicity in Coin Transfer and Error HandlingWhen transferring coins from the sender to the module account, ensure that the operation is atomic to prevent partial transfers in case of failures. The current implementation seems appropriate, but consider reviewing transaction atomicity for robustness.
x/evm/precompile/funtoken.go (1)
31-41: Clarify Gas Limit Calculation in CommentsThe comments provide gas estimates for each step, totaling approximately 325,000 gas:
- Step 1: Up to 200,000 gas
- Step 2: ~60,000 gas
- Step 3: ~65,000 gas
- Total: ~325,000 gas
However,
FunTokenGasLimitBankSendis set to 400,000 gas. Consider updating the comments to explain the additional 75,000 gas (perhaps accounting for overhead or gas discrepancies) or adjust the constant to better reflect the estimated total.x/evm/keeper/funtoken_from_erc20.go (1)
211-216: Add detailed function parameter documentationTo enhance readability and maintainability, consider adding GoDoc comments for each parameter of the
convertCoinNativeERC20function. This will help other developers understand the purpose and expected values of each parameter.x/evm/keeper/msg_server.go (1)
252-255: Clarify Documentation for New ParameterfullRefundLeftoverGasThe
ApplyEvmMsgfunction now includes the boolean parameterfullRefundLeftoverGas, which affects gas refund logic. Ensure that the function's documentation clearly explains the purpose and impact of this parameter. Providing detailed comments or examples can help other developers understand its intended use and avoid misuse.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
- CHANGELOG.md (1 hunks)
- x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json (1 hunks)
- x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol (1 hunks)
- x/evm/embeds/embeds.go (3 hunks)
- x/evm/embeds/embeds_test.go (1 hunks)
- x/evm/evmtest/test_deps.go (1 hunks)
- x/evm/keeper/erc20.go (7 hunks)
- x/evm/keeper/funtoken_from_coin.go (2 hunks)
- x/evm/keeper/funtoken_from_coin_test.go (2 hunks)
- x/evm/keeper/funtoken_from_erc20.go (2 hunks)
- x/evm/keeper/funtoken_from_erc20_test.go (5 hunks)
- x/evm/keeper/grpc_query.go (4 hunks)
- x/evm/keeper/keeper.go (2 hunks)
- x/evm/keeper/msg_server.go (5 hunks)
- x/evm/precompile/funtoken.go (3 hunks)
- x/evm/precompile/funtoken_test.go (5 hunks)
- x/evm/precompile/oracle.go (2 hunks)
- x/evm/precompile/oracle_test.go (2 hunks)
- x/evm/precompile/precompile.go (4 hunks)
- x/evm/precompile/test/export.go (4 hunks)
- x/evm/precompile/wasm.go (2 hunks)
- x/evm/precompile/wasm_test.go (5 hunks)
- x/evm/statedb/journal_test.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/evm/evmtest/test_deps.go
[warning] 64-66: x/evm/evmtest/test_deps.go#L64-L66
Added lines #L64 - L66 were not covered by testsx/evm/keeper/funtoken_from_coin.go
[warning] 104-105: x/evm/keeper/funtoken_from_coin.go#L104-L105
Added lines #L104 - L105 were not covered by tests
[warning] 107-108: x/evm/keeper/funtoken_from_coin.go#L107-L108
Added lines #L107 - L108 were not covered by tests
[warning] 149-150: x/evm/keeper/funtoken_from_coin.go#L149-L150
Added lines #L149 - L150 were not covered by tests
[warning] 152-154: x/evm/keeper/funtoken_from_coin.go#L152-L154
Added lines #L152 - L154 were not covered by testsx/evm/keeper/funtoken_from_erc20.go
[warning] 221-222: x/evm/keeper/funtoken_from_erc20.go#L221-L222
Added lines #L221 - L222 were not covered by tests
[warning] 224-225: x/evm/keeper/funtoken_from_erc20.go#L224-L225
Added lines #L224 - L225 were not covered by tests
[warning] 245-246: x/evm/keeper/funtoken_from_erc20.go#L245-L246
Added lines #L245 - L246 were not covered by tests
[warning] 248-249: x/evm/keeper/funtoken_from_erc20.go#L248-L249
Added lines #L248 - L249 were not covered by tests
[warning] 251-252: x/evm/keeper/funtoken_from_erc20.go#L251-L252
Added lines #L251 - L252 were not covered by tests
[warning] 263-264: x/evm/keeper/funtoken_from_erc20.go#L263-L264
Added lines #L263 - L264 were not covered by tests
[warning] 266-267: x/evm/keeper/funtoken_from_erc20.go#L266-L267
Added lines #L266 - L267 were not covered by tests
[warning] 272-273: x/evm/keeper/funtoken_from_erc20.go#L272-L273
Added lines #L272 - L273 were not covered by tests
[warning] 275-276: x/evm/keeper/funtoken_from_erc20.go#L275-L276
Added lines #L275 - L276 were not covered by tests
[warning] 280-281: x/evm/keeper/funtoken_from_erc20.go#L280-L281
Added lines #L280 - L281 were not covered by tests
[warning] 286-287: x/evm/keeper/funtoken_from_erc20.go#L286-L287
Added lines #L286 - L287 were not covered by testsx/evm/keeper/msg_server.go
[warning] 348-349: x/evm/keeper/msg_server.go#L348-L349
Added lines #L348 - L349 were not covered by tests
[warning] 591-592: x/evm/keeper/msg_server.go#L591-L592
Added lines #L591 - L592 were not covered by tests
[warning] 650-652: x/evm/keeper/msg_server.go#L650-L652
Added lines #L650 - L652 were not covered by tests
[warning] 665-669: x/evm/keeper/msg_server.go#L665-L669
Added lines #L665 - L669 were not covered by tests
[warning] 688-690: x/evm/keeper/msg_server.go#L688-L690
Added lines #L688 - L690 were not covered by tests
[warning] 695-696: x/evm/keeper/msg_server.go#L695-L696
Added lines #L695 - L696 were not covered by testsx/evm/precompile/funtoken.go
[warning] 94-95: x/evm/precompile/funtoken.go#L94-L95
Added lines #L94 - L95 were not covered by testsx/evm/precompile/oracle.go
[warning] 61-62: x/evm/precompile/oracle.go#L61-L62
Added lines #L61 - L62 were not covered by testsx/evm/precompile/precompile.go
[warning] 238-239: x/evm/precompile/precompile.go#L238-L239
Added lines #L238 - L239 were not covered by testsx/evm/precompile/wasm.go
[warning] 72-73: x/evm/precompile/wasm.go#L72-L73
Added lines #L72 - L73 were not covered by tests
🔇 Additional comments (51)
x/evm/embeds/embeds_test.go (1)
19-19: LGTM! The test addition aligns with PR objectives.The new test case follows the established pattern and verifies the loading of the
TestFunTokenPrecompileLocalGascontract, which is essential for testing the new local gas meter functionality.Let's verify the contract's existence:
✅ Verification successful
Contract verification successful - both source and artifact files exist
The verification confirms that both required files are present in the expected locations:
- Source:
x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol- Artifact:
x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the TestFunTokenPrecompileLocalGas contract exists # Expected: Find the contract file and its corresponding JSON artifact # Test 1: Find the contract file fd "TestFunTokenPrecompileLocalGas.sol" --type f # Test 2: Find the contract's JSON artifact fd "TestFunTokenPrecompileLocalGas.json" --type fLength of output: 260
x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol (1)
4-4: Verify the imported constant usage.The contract uses
FUNTOKEN_PRECOMPILE_ADDRESSbut it's not clear where this constant is defined.✅ Verification successful
Let me gather more information about the constant definition.
Import is correctly used and constant is properly defined
The constant
FUNTOKEN_PRECOMPILE_ADDRESSis defined inFunToken.solas0x0000000000000000000000000000000000000800and is properly used inTestFunTokenPrecompileLocalGas.solfor making precompile calls. The import statement is necessary and valid.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for FUNTOKEN_PRECOMPILE_ADDRESS definition rg -l "FUNTOKEN_PRECOMPILE_ADDRESS" # Search for the constant's value in FunToken.sol ast-grep --pattern 'constant FUNTOKEN_PRECOMPILE_ADDRESS = $_'Length of output: 194
Script:
#!/bin/bash # Show the content around FUNTOKEN_PRECOMPILE_ADDRESS in both files rg -B2 -A2 "FUNTOKEN_PRECOMPILE_ADDRESS" x/evm/embeds/contracts/FunToken.sol x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.solLength of output: 1484
x/evm/evmtest/test_deps.go (1)
64-67: Ensure test coverage for the new ResetGasMeter method.While this method is part of test utilities and might be indirectly tested through its usage in funtoken tests, consider adding direct unit tests to verify its behavior.
Would you like me to help create unit tests for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 64-66: x/evm/evmtest/test_deps.go#L64-L66
Added lines #L64 - L66 were not covered by testsx/evm/precompile/oracle_test.go (1)
64-69: Verify the purpose of thefalseparameter.The boolean parameter (line 67) seems to be related to gas limit usage. Please verify its purpose and consider adding a comment explaining what this boolean controls, as it's not immediately clear from the context.
✅ Verification successful
From the search results, I found the documentation of
CallContractWithInputinx/evm/keeper/msg_server.gowhich shows that the boolean parameter is namedcommitand is used to determine whether the contract call should be committed or simulated. Looking at the usage patterns across the codebase:
- For query operations (like in oracle_test.go),
commitis set tofalse- For state-changing operations (like in other tests),
commitis set totrueThe
falsevalue in the oracle test is correct because it's performing a query operation (queryExchangeRate) which doesn't modify state.The
falseparameter is correctly used for this query operation.The boolean parameter controls whether the contract call should commit changes (true) or just simulate/query (false). In this case, since it's a query to get exchange rate data, using
falseis the appropriate choice.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the CallContractWithInput definition to understand the boolean parameter ast-grep --pattern 'func $_ CallContractWithInput($_, $_, $_, $useGasLimit bool, $_, $_)'Length of output: 91
Script:
#!/bin/bash # Let's try a broader search for CallContractWithInput function definition rg -A 5 "CallContractWithInput" # Also search for any comments or documentation about this function rg -B 2 -A 5 "CallContractWithInput"Length of output: 13478
x/evm/precompile/oracle.go (3)
41-45: LGTM! Clear variable naming improvement.The rename from 'res' to 'start' better reflects the initialization nature of the
OnRunStartfunction.
47-48: Excellent gas meter cleanup implementation!The deferred reset of the gas meter is a crucial safety feature that ensures:
- Proper cleanup of gas meter state
- Graceful handling of "out of gas" scenarios
- Consistent gas tracking across precompile executions
58-62: Add tests and documentation for gas consumption check.This critical gas consumption check needs:
- Test coverage to verify proper handling of out-of-gas scenarios
- A comment explaining the purpose and importance of this check
Would you like me to help:
- Generate test cases for the gas consumption check?
- Draft documentation for this security-critical feature?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 61-62: x/evm/precompile/oracle.go#L61-L62
Added lines #L61 - L62 were not covered by testsx/evm/embeds/embeds.go (3)
32-33: LGTM! Embedded JSON declaration follows established patterns.The new embedded JSON variable follows the package's conventions for naming and file structure.
97-97: LGTM! Contract loading follows established patterns.The initialization is correctly implemented using
MustLoad(), maintaining consistency with other contract loading.
32-33: Verify the contract's integration and test coverage.Let's ensure the contract is properly integrated and tested:
Also applies to: 81-86
✅ Verification successful
Let me gather more information about the test files to better understand the integration.
Let me check the actual Solidity contract implementation to complete the verification.
Based on the comprehensive verification results, I can now provide the final response:
Contract is properly integrated and well-tested ✅
The verification confirms:
- The Solidity contract exists and implements gas-specific test scenarios for the FunToken precompile
- The contract is properly used in test files:
x/evm/precompile/funtoken_test.go: Tests both default and custom gas scenariosx/evm/embeds/embeds_test.go: Verifies contract loading- The implementation aligns with its intended purpose of testing gas limits in precompiles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify contract integration and test coverage # Test 1: Check if the JSON file exists echo "Checking for contract JSON file..." fd -t f "TestFunTokenPrecompileLocalGas.json" artifacts/ # Test 2: Look for test files using this contract echo "Checking for test files using this contract..." rg -l "TestFunTokenPrecompileLocalGas" -g "**/*_test.go" # Test 3: Check for actual contract usage echo "Checking for contract usage in codebase..." rg "SmartContract_TestFunTokenPrecompileLocalGas" -g "!**/embeds.go"Length of output: 994
Script:
#!/bin/bash # Check the test implementations to understand how the contract is used echo "Checking funtoken_test.go implementation..." rg -A 5 -B 5 "TestFunTokenPrecompileLocalGas" x/evm/precompile/funtoken_test.go echo -e "\nChecking embeds_test.go implementation..." rg -A 5 -B 5 "TestFunTokenPrecompileLocalGas" x/evm/embeds/embeds_test.go # Check if there's a corresponding Solidity contract echo -e "\nLooking for the Solidity contract..." fd -t f "TestFunTokenPrecompileLocalGas.sol"Length of output: 1826
Script:
#!/bin/bash # Check the Solidity contract implementation echo "Checking Solidity contract implementation..." cat x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol # Verify if the contract is properly loaded in embeds.go echo -e "\nChecking contract loading in embeds.go..." rg -A 5 "var SmartContract_TestFunTokenPrecompileLocalGas" x/evm/embeds/embeds.goLength of output: 1748
x/evm/keeper/keeper.go (2)
7-7: LGTM: Import change looks correct.The import of
cosmossdk.io/mathis appropriate as it's used by theGetMinGasUsedMultipliermethod.
112-116: LGTM: Method rename and documentation improvements.The rename to
GetMinGasUsedMultiplierbetter reflects the method's purpose, and the documentation clearly explains how the gas limit and multiplier interact.x/evm/statedb/journal_test.go (2)
12-13: LGTM: Import added for accessing gas limit constants.The keeper package import is correctly added to support the gas limit parameter changes.
70-75: Verify gas limit appropriateness for test scenarios.The addition of
keeper.Erc20GasLimitExecuteas the gas limit parameter aligns with the PR objectives. However, we should ensure this limit is appropriate for the test scenario and consistent across similar test cases.Let's verify the consistency of gas limit usage across test files:
✅ Verification successful
Let me analyze the results and generate a final response based on the findings.
Gas limit parameter is consistently used across the codebase
The verification shows that
CallContractWithInputis properly used with appropriate gas limits across the codebase:
- ERC20 operations use
Erc20GasLimitExecutein keeper operations- Test files correctly use the gas limit parameter:
journal_test.gousesErc20GasLimitExecutefor ERC20 operationsfuntoken_test.gousesErc20GasLimitQueryfor queries- Precompile tests (
oracle_test.go,wasm_test.go) have their own gas limit configurationsThe gas limit parameter addition is appropriate for the test scenario and maintains consistency with the production code patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other test files that might need similar gas limit updates # and verify the consistency of CallContractWithInput usage. # Search for other test files using CallContractWithInput echo "Files containing CallContractWithInput calls that might need gas limit updates:" rg -l "CallContractWithInput" --type go --type-add 'go:*.go' | grep "_test.go" # Check the actual usage patterns echo -e "\nUsage patterns of CallContractWithInput:" rg -A 5 "CallContractWithInput\(" --type goLength of output: 6043
x/evm/keeper/erc20.go (6)
66-66: LGTM! Gas limit properly applied to mint operation.The addition of
Erc20GasLimitExecuteensures controlled gas consumption for minting operations.
88-88: LGTM! Gas limit properly applied to transfer operation.The addition of
Erc20GasLimitExecuteensures controlled gas consumption for transfer operations.
128-128: LGTM! Gas limit properly applied to burn operation.The addition of
Erc20GasLimitExecuteensures controlled gas consumption for burn operations.
157-163: LGTM! Query gas limit properly applied to string loading operations.The addition of
Erc20GasLimitQueryis appropriate for these read-only operations.
189-191: LGTM! Query gas limit properly applied to uint8 loading operations.The addition of
Erc20GasLimitQueryis appropriate for these read-only operations.
220-220: LGTM! Query gas limit properly applied to BigInt loading operations.The addition of
Erc20GasLimitQueryis appropriate for these read-only operations.x/evm/precompile/funtoken_test.go (2)
11-12: LGTM! Well-structured test suite modifications.The addition of
depsandfuntokenfields toFuntokenSuiteimproves test organization and follows good practices for dependency injection.Also applies to: 30-32
34-40: LGTM! Clean and well-documented test setup.The
SetupSuiteimplementation properly initializes test dependencies and includes helpful logging.x/evm/precompile/wasm_test.go (3)
62-67: LGTM! Gas limit properly applied to denom creation call.The gas limit is appropriately set for the contract execution, and error handling is in place.
333-338: LGTM! Consistent gas limit usage in error test cases.The execution gas limit is consistently applied even in error test cases, which is a good practice for thorough testing.
155-163: LGTM! Verify gas meter reset effectiveness.The gas meter reset and query gas limit are properly implemented. The lower gas limit for queries is appropriate for read-only operations.
Let's verify that the gas meter reset is working as expected:
✅ Verification successful
Gas meter reset is consistently used before gas-sensitive operations
The verification shows that
ResetGasMeter()is consistently used before EVM contract calls and other gas-sensitive operations across test files. The implementation inevmtest/test_deps.goproperly resets both transient and regular gas meters, ensuring accurate gas measurements in tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if ResetGasMeter is consistently used before gas-sensitive operations # across the codebase to ensure accurate gas measurements in tests. rg -B 2 -A 2 'ResetGasMeter\(\)' --type goLength of output: 2997
x/evm/keeper/funtoken_from_coin_test.go (2)
260-261: LGTM: Proper gas meter reset before conversion.The addition of
ResetGasMeter()ensures accurate gas measurement for the ERC-20 to bank coin conversion operation by starting with a clean gas state.
269-269: LGTM: Gas limit enforcement for bankSend operation.The addition of
precompile.FunTokenGasLimitBankSendimplements proper gas management for the bankSend operation, aligning with the PR's objective of introducing local gas meters in precompiles.Let's verify the gas limit value is appropriate:
Also applies to: 297-297
✅ Verification successful
Let me gather more context about the gas limit usage in the code.
Gas limit value for bankSend is appropriately set
The
FunTokenGasLimitBankSendvalue of 400,000 gas is well-justified based on the code comments and comparison with other gas limits in the codebase:
- It accounts for ~65,000 gas for bank send operation
- Includes buffer for additional operations (mint/burn ~60,000 gas each)
- Aligns proportionally with other precompile gas limits (e.g., Erc20GasLimitExecute: 200,000, WasmGasLimitQuery: 200,000)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the FunTokenGasLimitBankSend constant value and its usage # Check the constant definition rg -A 1 "FunTokenGasLimitBankSend.*=.*" # Check all usages of this constant rg "FunTokenGasLimitBankSend"Length of output: 1014
Script:
#!/bin/bash # Get the implementation details around FunTokenGasLimitBankSend ast-grep --pattern 'const FunTokenGasLimitBankSend = $_' # Get more context around the constant definition and its comments rg -B 3 -A 3 "FunTokenGasLimitBankSend.*=.*" # Check if there are any other gas limits defined for comparison rg "GasLimit.*=.*\d+"Length of output: 2217
x/evm/precompile/test/export.go (2)
80-85: LGTM! Gas limit properly applied to contract instantiation.The WasmGasLimitInstantiate constant is correctly used in the CallContractWithInput method.
326-334: Consider scaling gas limit with message count.While the gas management is generally good with the reset and appropriate limit, the gas limit doesn't scale with the number of messages being executed.
Consider adjusting the gas limit based on the number of messages:
- WasmGasLimitExecute, + WasmGasLimitExecute * uint64(times),Let's verify if there are any existing tests that might fail with multiple messages:
x/evm/precompile/wasm.go (2)
69-73:⚠️ Potential issueAdd test coverage for gas consumption checks.
The gas consumption check logic is correct, but lacks test coverage according to static analysis. This is critical functionality that should be thoroughly tested.
Test cases should cover:
- Normal case: Contract has sufficient gas
- Edge case: Contract runs out of gas
- Integration with parent gas meter
Let's verify existing gas-related test coverage:
#!/bin/bash # Search for gas-related test cases in precompile tests rg -g "**/*_test.go" -B 5 -A 5 "UseGas|GasConsumed|ErrOutOfGas" x/evm/precompile/Would you like me to help generate comprehensive test cases for this functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 72-73: x/evm/precompile/wasm.go#L72-L73
Added lines #L72 - L73 were not covered by tests
45-46: LGTM: Gas meter reset implementation looks correct.The deferred gas meter reset ensures proper cleanup after precompile execution and handles "out of gas" errors gracefully.
Let's verify the
ReturnToParentGasMeterimplementation:✅ Verification successful
Gas meter reset implementation is correctly implemented and used
The verification confirms that
ReturnToParentGasMeteris properly implemented inx/evm/precompile/precompile.goand correctly used across multiple precompiles. The implementation:
- Resets the context gas meter back to parent after precompile execution
- Properly handles out of gas panics by catching
storetypes.ErrorOutOfGas- Correctly updates the contract's gas usage
- Is consistently used in all precompile implementations (wasm.go, funtoken.go, oracle.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ReturnToParentGasMeter implementation and usages rg -A 5 "ReturnToParentGasMeter"Length of output: 83
Script:
#!/bin/bash # Let's try a broader search for gas meter related functions and their implementations rg -A 10 "func.*GasMeter|ReturnToParentGasMeter" # Also search for the specific file content cat x/evm/precompile/wasm.goLength of output: 21827
x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json (2)
5-57: LGTM: Contract interface properly implements gas management requirements.The ABI definition correctly implements:
- Constructor with ERC20 address parameter
- Base function
callBankSendfor standard calls- Extended function
callBankSendLocalGaswith custom gas parameter
59-60: Verify the bytecode's gas management implementation.The bytecode includes proper error handling for failed calls and uses the correct precompile interaction pattern. However, let's verify the gas forwarding implementation in the source contract.
✅ Verification successful
Based on the source code verification, I can now generate the final response:
Gas management implementation is correctly implemented
The bytecode correctly reflects the source contract's gas management implementation where:
callBankSend()forwards all available gas (default behavior)callBankSendLocalGas()allows custom gas limit via thegasparameter- Both functions include proper error handling with require statements
- No other hardcoded gas values were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the source contract to verify gas forwarding implementation fd -t f "TestFunTokenPrecompileLocalGas.sol" | xargs cat # Verify no hardcoded gas values in other precompile calls rg -g "*.sol" "gas:" -A 5Length of output: 2077
x/evm/keeper/funtoken_from_erc20_test.go (2)
205-205: LGTM: Consistent gas limit implementationThe gas limits are appropriately set for different operations:
Erc20GasLimitExecutefor ERC20 mint operationsFunTokenGasLimitBankSendfor bank send operationsThis aligns well with the PR's objective of enhancing gas management in precompiles.
Also applies to: 223-223, 247-247, 379-379
214-215: LGTM: Strategic gas meter resetsGas meters are appropriately reset before major operations to ensure accurate gas measurement in each test case. The placement is consistent and helps isolate gas consumption for individual operations.
Also applies to: 238-239, 256-257, 370-371
x/evm/keeper/grpc_query.go (3)
287-287: LGTM: Consistent with non-committing behavior.The addition of
falseparameter aligns with the comment above about not committing StateDB.
425-425: LGTM: Appropriate for gas estimation.The
falseparameter is correctly added to maintain the non-committing nature of gas estimation.
521-521: LGTM: Suitable for transaction tracing.The
falseparameter is correctly added to maintain the non-committing nature of transaction tracing.CHANGELOG.md (1)
Line range hint
1-1338: LGTM! Well-structured changelog following best practices.The changelog is well-organized and follows the Keep a Changelog format consistently. It effectively documents changes with:
- Clear sections for different types of changes
- Consistent PR link formatting
- Chronological ordering
- Good use of markdown for readability
x/evm/keeper/funtoken_from_coin.go (5)
4-4: Approved: Proper Import of"context"PackageThe import of the
"context"package is necessary for usingcontext.Contextin the new methodConvertCoinToEvm.
96-117: Review Error Handling inConvertCoinToEvmMethodThe method
ConvertCoinToEvmproperly handles various error cases such as missing or multiple FunToken mappings. Ensure that all possible error paths are adequately tested.Would you like help in identifying any untested error paths within this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 104-105: x/evm/keeper/funtoken_from_coin.go#L104-L105
Added lines #L104 - L105 were not covered by tests
[warning] 107-108: x/evm/keeper/funtoken_from_coin.go#L107-L108
Added lines #L107 - L108 were not covered by tests
156-160: Approved: Event Emission After Successful ConversionEmitting the
EventConvertCoinToEvmevent after a successful conversion is good practice for transparency and auditing.
83-83: Verify the Use ofErc20GasLimitDeployin Contract DeploymentEnsure that the
Erc20GasLimitDeployparameter is correctly defined and passed to thedeployERC20ForBankCoinfunction. This parameter controls the gas limit for contract deployment and should be consistently used across the codebase.Run the following script to confirm that
Erc20GasLimitDeployis appropriately defined and used:#!/bin/bash # Description: Verify usage of Erc20GasLimitDeploy in the codebase. # Test: Search for the definition and usage of Erc20GasLimitDeploy. # Expect: Erc20GasLimitDeploy is defined and used where necessary. rg 'Erc20GasLimitDeploy'
143-143: Confirm Definition ofErc20GasLimitExecuteThe variable
Erc20GasLimitExecuteused in theCallContractfunction should be defined and set appropriately. Verify that it holds the correct gas limit value for executing contracts.Run the following script to check the definition and usage of
Erc20GasLimitExecute:✅ Verification successful
Based on the search results, I can generate the final response as we have found clear evidence of the definition and usage of
Erc20GasLimitExecute.
Erc20GasLimitExecuteis properly defined with appropriate gas limitThe constant is well-defined in
x/evm/keeper/erc20.gowith a value of 200,000 gas units and is documented as being used for transfer, mint, and burn operations. This is a reasonable gas limit for ERC20 operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify definition and usage of Erc20GasLimitExecute. # Test: Search for the definition and ensure it's correctly assigned. # Expect: Erc20GasLimitExecute is defined with an appropriate gas limit. rg 'Erc20GasLimitExecute'Length of output: 725
x/evm/precompile/precompile.go (2)
151-152: Looks good: Addition ofparentGasMeterfieldThe inclusion of
parentGasMeterin theOnRunStartResultstruct is appropriate for managing gas meters during precompile execution.
199-205: Verify thatReturnToParentGasMeteris properly invokedAfter switching to a local gas meter within
OnRunStart, please ensure thatReturnToParentGasMeteris called appropriately to restore the parent gas meter after execution or in case of errors. This is crucial for maintaining accurate gas accounting.x/evm/precompile/funtoken.go (1)
74-76: Proper Handling of Gas Meter ResetThe deferred call to
ReturnToParentGasMetercorrectly ensures that the gas meter is reset to the parent gas meter after precompile execution and gracefully handles "out of gas" errors. This implementation enhances the reliability of gas management within the precompile.x/evm/keeper/funtoken_from_erc20.go (3)
296-296: Ensure consistent return values on errorsVerify that all error paths return
nilfor the response to maintain consistency and prevent potential nil pointer dereferences.Please confirm that in all error scenarios, the function returns
nilfor the*evm.MsgConvertCoinToEvmResponseto ensure consistency.
245-249: 🛠️ Refactor suggestionSimplify EVM module balance error handling
Similar to the previous comment, consider simplifying the error handling for
evmModuleBalanceto improve code clarity.Suggested change:
evmModuleBalance, err := k.ERC20().BalanceOf( erc20Addr, evm.EVM_MODULE_ADDRESS, ctx, ) if err != nil { return nil, errors.Wrap(err, "failed to retrieve EVM module balance") } - if evmModuleBalance == nil { - return nil, fmt.Errorf("failed to retrieve balance, balance is nil") - }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 245-246: x/evm/keeper/funtoken_from_erc20.go#L245-L246
Added lines #L245 - L246 were not covered by tests
[warning] 248-249: x/evm/keeper/funtoken_from_erc20.go#L248-L249
Added lines #L248 - L249 were not covered by tests
270-276: 🛠️ Refactor suggestionSimplify recipient balance after transfer error handling
Consistent with prior suggestions, streamline the error checks for
recipientBalanceAfter.Suggested change:
recipientBalanceAfter, err := k.ERC20().BalanceOf(erc20Addr, recipient, ctx) if err != nil { return nil, errors.Wrap(err, "failed to retrieve recipient balance after transfer") } - if recipientBalanceAfter == nil { - return nil, fmt.Errorf("failed to retrieve balance, balance is nil") - }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 272-273: x/evm/keeper/funtoken_from_erc20.go#L272-L273
Added lines #L272 - L273 were not covered by tests
[warning] 275-276: x/evm/keeper/funtoken_from_erc20.go#L275-L276
Added lines #L275 - L276 were not covered by testsx/evm/keeper/msg_server.go (1)
66-66: Ensure Consistent Usage of New Parameter inApplyEvmMsgAt line 66, the
ApplyEvmMsgfunction is called with the new parameterfalse. Please verify that all other invocations ofApplyEvmMsgthroughout the codebase have been updated to include the newfullRefundLeftoverGasparameter to maintain consistency and prevent potential issues.You can run the following script to identify all usages of
ApplyEvmMsgand check for correct parameter usage:✅ Verification successful
Based on the
rgsearch results, I can generate the final response as all instances ofApplyEvmMsgcalls are visible with their parameters.All
ApplyEvmMsgCalls Are Consistent with New ParameterAll invocations of
ApplyEvmMsgthroughout the codebase have been properly updated to include the newfullRefundLeftoverGasparameter:
x/evm/keeper/msg_server.go:66:falsefor regular EVM transactionsx/evm/keeper/msg_server.go:144:truefor internal callsx/evm/keeper/grpc_query.go:52:falsefor query operationsx/evm/keeper/grpc_query.go:108:falsefor estimationx/evm/keeper/grpc_query.go:143:falsefor trace operationsx/evm/keeper/grpc_query.go:171:falsefor raw call operations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all calls to ApplyEvmMsg and display the surrounding lines. # Expectation: All calls should include the new parameter. rg 'ApplyEvmMsg\(' -A 3 -B 3Length of output: 3408
x/evm/precompile/test/export.go
Outdated
| deps.Ctx, | ||
| deps.Sender.EthAddr, | ||
| &precompile.PrecompileAddr_Wasm, | ||
| true, | ||
| input, | ||
| WasmGasLimitInstantiate, |
There was a problem hiding this comment.
Consider using a separate gas limit constant for queries.
Currently using WasmGasLimitInstantiate for a query operation, which is excessive as queries typically consume less gas than contract instantiation.
Consider introducing a new constant like WasmGasLimitQuery with a lower value appropriate for query operations:
const (
WasmGasLimitInstantiate uint64 = 1_000_000
WasmGasLimitExecute uint64 = 10_000_000
+ WasmGasLimitQuery uint64 = 100_000
)Then update the query operation:
- WasmGasLimitInstantiate,
+ WasmGasLimitQuery,Committable suggestion was skipped due to low confidence.
x/evm/keeper/msg_server.go
Outdated
| return nil, fmt.Errorf("failed to pack ABI args: %w", err) | ||
| } |
There was a problem hiding this comment.
Add Test Coverage for ABI Packing Failure
Lines 591-592 handle errors that may occur during ABI argument packing. Consider adding test cases that simulate a packing failure to verify that the error is correctly caught and handled, ensuring robustness in contract interactions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 591-592: x/evm/keeper/msg_server.go#L591-L592
Added lines #L591 - L592 were not covered by tests
x/evm/keeper/msg_server.go
Outdated
| err = errors.Wrapf(err, "failed to load EVM config") | ||
| return | ||
| } |
There was a problem hiding this comment.
Add Test Coverage for EVM Config Loading Failure
The error handling at lines 650-652 addresses failures in loading the EVM configuration. Adding unit tests that simulate this failure scenario can help ensure that the system responds appropriately when such an error occurs.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 650-652: x/evm/keeper/msg_server.go#L650-L652
Added lines #L650 - L652 were not covered by tests
x/evm/keeper/msg_server.go
Outdated
| // We don't know the actual gas used, so consuming the gas limit | ||
| k.ResetGasMeterAndConsumeGas(ctx, gasLimit) | ||
| err = errors.Wrap(err, "failed to apply ethereum core message") | ||
| return | ||
| } |
There was a problem hiding this comment.
Add Test Coverage for EVM Message Application Failure
Lines 665-669 deal with failures when applying the EVM message. Implementing tests to cover this failure path will help validate that the system handles such errors gracefully and that the correct amount of gas is consumed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 665-669: x/evm/keeper/msg_server.go#L665-L669
Added lines #L665 - L669 were not covered by tests
x/evm/keeper/msg_server.go
Outdated
| k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit()) | ||
| return nil, nil, errors.Wrap(err, "error adding transient gas used to block") | ||
| } |
There was a problem hiding this comment.
Add Test Coverage for Block Gas Usage Update Failure
At lines 688-690, there's error handling for failures in updating the block's gas usage. It's important to include tests that cover this scenario to ensure that any issues in gas accounting are detected and managed correctly.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 688-690: x/evm/keeper/msg_server.go#L688-L690
Added lines #L688 - L690 were not covered by tests
x/evm/keeper/msg_server.go
Outdated
| return nil, nil, errors.Wrap(err, "error emitting ethereum tx events") | ||
| } |
There was a problem hiding this comment.
Add Test Coverage for Event Emission Failure
Lines 695-696 address potential errors when emitting Ethereum transaction events. Considering the importance of accurate event logs, adding tests for this failure scenario will help ensure reliable system behavior and easier troubleshooting.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 695-696: x/evm/keeper/msg_server.go#L695-L696
Added lines #L695 - L696 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
x/evm/keeper/funtoken_from_coin.go (1)
82-82: Document the gas limit parameter.
Consider adding a comment explaining the expected gas limit range and any constraints for ERC20 deployment.
Add a comment above the CallContractWithInput call:
+ // Erc20GasLimitDeploy should be sufficient for standard ERC20 deployment
+ // Typical deployment costs: [ADD TYPICAL GAS COST HERE]
_, _, err = k.CallContractWithInput(x/evm/keeper/call_contract.go (2)
62-63: Update function comment to reflect custom gas limit
The comment states: "It uses a default gas limit." However, the function now accepts a gasLimit parameter, allowing callers to specify a custom gas limit. Please update the comment to reflect this change.
81-81: Rename unusedBigInt to zeroBigInt for clarity
The variable unusedBigInt is actively used as a zero value for amount, gasFeeCap, gasTipCap, and gasPrice in the evmMsg. Since it is not unused, consider renaming it to zeroBigInt or bigIntZero to enhance code readability and accurately represent its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- x/evm/keeper/call_contract.go (1 hunks)
- x/evm/keeper/funtoken_from_coin.go (1 hunks)
- x/evm/keeper/msg_server.go (5 hunks)
🔇 Additional comments (2)
x/evm/keeper/funtoken_from_coin.go (1)
82-82: Verify gas limit sufficiency for ERC20 deployment.
The addition of Erc20GasLimitDeploy improves gas management control. However, we should ensure the gas limit is sufficient for all ERC20 deployment scenarios.
Let's analyze the gas usage patterns in existing deployments:
✅ Verification successful
Based on the search results, I found clear evidence about the gas limit in x/evm/keeper/erc20.go:
// Erc20GasLimitDeploy only used internally when deploying ERC20Minter.
// Deployment requires ~1_600_000 gas
Erc20GasLimitDeploy uint64 = 2_000_000Let's verify if this gas limit has been sufficient in practice by checking the test cases:
Let me try a different search pattern to find any test cases or issues related to ERC20 deployment:
Based on the evidence gathered, I can now provide a final response:
Gas limit of 2M for ERC20 deployment is well-calibrated
The Erc20GasLimitDeploy value of 2,000,000 gas is appropriate because:
- Internal documentation confirms deployment requires ~1.6M gas
- The 2M limit provides a 25% safety margin above the required amount
- No gas-related failures were found in the test suite
- The gas limit is used consistently across contract deployments via
CallContractWithInput
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing ERC20 deployment gas usage patterns
rg -A 5 "deploy.*ERC20"
Length of output: 6995
Script:
#!/bin/bash
# Search for any gas-related failures or issues in ERC20 tests
rg -i "gas.*fail.*erc20|erc20.*gas.*fail"
# Check test files for any gas-related configurations
rg "gas.*erc20|erc20.*gas" "**/*test*"
Length of output: 134
Script:
#!/bin/bash
# Search for ERC20 deployment tests and any gas-related configurations
rg -A 5 "TestERC20.*deploy|test.*deploy.*ERC20"
# Search for any gas-related constants or configurations in ERC20 files
rg "gas.*=|gasLimit" x/evm/keeper/
Length of output: 2608
x/evm/keeper/msg_server.go (1)
66-66: Verify consistent usage of fullRefundLeftoverGas parameter across codebase.
In this call to ApplyEvmMsg, the fullRefundLeftoverGas parameter is set to false. Please ensure that all calls to ApplyEvmMsg pass the appropriate value for fullRefundLeftoverGas based on the context, to maintain consistent gas refund behavior.
Run the following script to find all calls to ApplyEvmMsg and display the values passed for fullRefundLeftoverGas:
✅ Verification successful
Let me get more context by checking the actual calls to ApplyEvmMsg with surrounding code.
fullRefundLeftoverGas parameter is used consistently across the codebase
The parameter is used consistently based on the context:
- Set to
falsefor all regular EVM transactions and queries - Set to
trueonly incall_contract.gofor internal contract calls, where the comment explicitly states "For internal calls like funtokens, user does not specify gas limit explicitly. In this case we don't apply any caps for refund and refund 100%"
The current change setting it to false in msg_server.go aligns with this pattern since it handles regular EVM transactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to ApplyEvmMsg and extract the `fullRefundLeftoverGas` argument.
# Test: Search for ApplyEvmMsg function calls and list the values of `fullRefundLeftoverGas`.
rg --no-filename --no-line-number 'ApplyEvmMsg\([^)]*\)' -t go \
| grep -o 'ApplyEvmMsg([^)]*)' \
| sed -E 's/.*ApplyEvmMsg\(([^)]*)\).*/\1/' \
| awk -F',' '{print $NF}' \
| sed 's/^[ \t]*//;s/[ \t]*$//' \
| sort | uniq -c
Length of output: 253
Script:
#!/bin/bash
# Search for ApplyEvmMsg calls with context
rg "ApplyEvmMsg\(" -B 2 -A 2 -t go
Length of output: 2480
| // | ||
| // # fullRefundLeftoverGas parameter | ||
| // For internal calls like funtokens, user does not specify gas limit explicitly. | ||
| // In this case we don't apply any caps for refund and refund 100% |
There was a problem hiding this comment.
Include fullRefundLeftoverGas in the function's documentation.
The parameter fullRefundLeftoverGas is introduced in the ApplyEvmMsg function but is not described in the function's doc comment. To improve clarity and maintain comprehensive documentation, please include a description of the fullRefundLeftoverGas parameter within the function's documentation. This will help other developers understand its purpose and usage.
| refundQuotient := params.RefundQuotientEIP3529 // EIP-3529: refunds are capped to gasUsed / 5 | ||
| minGasUsedPct := k.GetMinGasUsedMultiplier(ctx) // Evmos invention: https://github.com/evmos/ethermint/issues/1085 | ||
| if fullRefundLeftoverGas { | ||
| refundQuotient = 1 // 100% refund | ||
| minGasUsedPct = math.LegacyZeroDec() // no minimum, get the actual gasUsed value |
There was a problem hiding this comment.
Add unit tests for fullRefundLeftoverGas logic.
The conditional logic based on fullRefundLeftoverGas affects the refund calculation by altering refundQuotient and minGasUsedPct. To ensure correctness and prevent regressions, please add unit tests that cover both scenarios where fullRefundLeftoverGas is true and false. This will verify that the refund calculations behave as expected in both cases.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
x/evm/precompile/funtoken.go (1)
30-40: LGTM! Consider documenting the buffer allocation.The gas limit breakdown is well documented and the calculations are reasonable. The total limit of 400,000 provides a ~75,000 gas buffer above the estimated ~325,000 (200,000 + 60,000 + 65,000).
Consider adding a comment about the buffer allocation to help future maintainers understand the safety margin:
// 3. send from module to account: // ~65_000 gas (bank send) +// Buffer: ~75,000 gas for safety margin FunTokenGasLimitBankSend uint64 = 400_000x/evm/precompile/test/export.go (1)
24-28: LGTM! Consider enhancing the documentation.The gas limit constants are well-defined with reasonable values for their respective operations. However, the comment could be more descriptive.
Consider updating the comment to be more specific:
-// rough gas limits for wasm execution - used in tests only +// Gas limits for Wasm contract operations in tests. +// WasmGasLimitInstantiate: Gas limit for contract instantiation +// WasmGasLimitExecute: Gas limit for contract execution
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
CHANGELOG.md(1 hunks)x/evm/embeds/embeds.go(3 hunks)x/evm/embeds/embeds_test.go(1 hunks)x/evm/evmtest/test_deps.go(1 hunks)x/evm/keeper/erc20.go(7 hunks)x/evm/keeper/funtoken_from_coin_test.go(4 hunks)x/evm/keeper/grpc_query.go(4 hunks)x/evm/keeper/keeper.go(2 hunks)x/evm/keeper/msg_server.go(5 hunks)x/evm/precompile/funtoken.go(3 hunks)x/evm/precompile/funtoken_test.go(6 hunks)x/evm/precompile/oracle.go(1 hunks)x/evm/precompile/precompile.go(5 hunks)x/evm/precompile/test/export.go(4 hunks)x/evm/precompile/wasm.go(2 hunks)x/evm/precompile/wasm_test.go(7 hunks)x/evm/statedb/journal_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- x/evm/embeds/embeds.go
- x/evm/embeds/embeds_test.go
- x/evm/evmtest/test_deps.go
- x/evm/keeper/erc20.go
- x/evm/keeper/funtoken_from_coin_test.go
- x/evm/keeper/keeper.go
- x/evm/keeper/msg_server.go
- x/evm/precompile/wasm.go
- x/evm/precompile/wasm_test.go
- x/evm/statedb/journal_test.go
🔇 Additional comments (15)
x/evm/precompile/oracle.go (2)
45-48: LGTM: Enhanced gas management with proper initialization.
The addition of contract.Gas parameter to OnRunStart properly initializes the gas context for the precompile execution.
51-52: Verify gas meter management implementation.
The deferred gas meter reset looks correct, but let's verify the implementation of ReturnToParentGasMeter to ensure it properly handles all edge cases.
✅ Verification successful
The implementation and usage analysis shows that ReturnToParentGasMeter is well-implemented with proper error handling for out-of-gas scenarios and is consistently used across multiple precompiles (oracle.go, funtoken.go, wasm.go). Let me verify one more aspect to ensure complete coverage of edge cases.
Gas meter management implementation is correctly implemented and safe
The implementation of ReturnToParentGasMeter is robust and handles edge cases appropriately:
- Properly recovers from panics and specifically handles out-of-gas errors
- Correctly transfers consumed gas to the contract when out-of-gas occurs
- Consistently used across all precompiles (oracle, funtoken, wasm)
- Safely interacts with both standard and custom gas meter implementations
- Re-panics for unexpected errors to maintain system integrity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of ReturnToParentGasMeter and its usage
# Expected: Find the implementation and other usages to confirm consistent behavior
# Search for the implementation
echo "Searching for ReturnToParentGasMeter implementation:"
rg -A 10 "func ReturnToParentGasMeter"
# Search for other usages to verify consistency
echo "Searching for other usages of ReturnToParentGasMeter:"
rg "ReturnToParentGasMeter"
Length of output: 1634
Script:
#!/bin/bash
# Check for any custom gas meter implementations or overrides that might interact with this
rg -A 5 "type.*GasMeter.*struct"
# Look for any panic recovery or error handling patterns in the precompile package
rg "panic\(" x/evm/precompile/
rg "recover\(" x/evm/precompile/
Length of output: 809
x/evm/precompile/funtoken_test.go (3)
11-12: LGTM: Clean structural improvements.
The addition of deps and funtoken fields to the FuntokenSuite struct improves test organization by centralizing test dependencies and avoiding repeated setup.
Also applies to: 30-32
34-40: LGTM: Well-structured test setup.
The SetupSuite implementation follows testing best practices with:
- Clear initialization of test dependencies
- Helpful logging for debugging
- Proper use of test suite hooks
108-108: LGTM: Improved gas management in tests.
The changes enhance the test by:
- Using centralized funtoken instance from the suite
- Adding explicit gas limits for contract calls
- Including gas meter reset for accurate measurements
Also applies to: 115-115, 128-128, 159-159
x/evm/precompile/funtoken.go (3)
71-72: LGTM! Robust gas management implementation.
The implementation properly handles gas management with:
- Deferred reset of gas meter
- Accurate gas consumption tracking
- Proper error handling for out-of-gas scenarios
Also applies to: 88-92
77-77: LGTM! Proper integration with gas management.
The bankSend method is correctly integrated with the new gas management system.
Line range hint 30-92: Verify gas limit security implications.
Let's verify that the gas limit implementation cannot be exploited to bypass security checks or manipulate token transfers.
✅ Verification successful
Gas limit implementation is secure with proper safeguards
The verification reveals robust gas management with multiple security layers:
-
Fixed gas limits are consistently defined across precompiles with safe upper bounds:
FunTokenGasLimitBankSend(400k) aligns with individual operation costs- ERC20 operations capped at 200k gas
- Gas consumption tracked and validated at multiple levels
-
Key security measures in place:
- Proper gas metering with
GasConsumed()checks - Out-of-gas conditions handled explicitly
- Gas refund mechanisms with overflow protection
- Block-level gas limits enforced
- Proper gas metering with
The implementation follows best practices for gas management and includes appropriate bounds checking to prevent manipulation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential gas manipulation vulnerabilities
# Test 1: Search for other gas limit constants to ensure consistency
echo "Checking for gas limit constants..."
rg -A 2 "GasLimit.*uint64.*=.*_000"
# Test 2: Search for gas-related security checks
echo "Checking for gas-related security measures..."
rg -A 5 "UseGas|GasConsumed|OutOfGas"
# Test 3: Search for potential gas-related TODOs or FIXMEs
echo "Checking for gas-related TODOs..."
rg -i "todo.*gas|fixme.*gas"
Length of output: 13597
x/evm/precompile/precompile.go (2)
153-154: LGTM! Well-structured addition for gas management.
The new field parentGasMeter is appropriately added to store the original gas meter, enabling proper gas tracking and restoration.
185-185: Verify gas accounting implications.
The changes introduce two potential concerns:
- Setting empty gas configs (
WithKVGasConfigandWithTransientKVGasConfig) might bypass important gas accounting for storage operations. - The
gasLimitparameter is not validated, which could lead to unexpected behavior with zero or very large values.
Please verify:
- That bypassing KV gas costs is intentional and safe
- Add validation for the gasLimit parameter
Let's check if there are any tests covering these scenarios:
Also applies to: 209-215
✅ Verification successful
Let me gather more information about the gas metering implementation and validation:
Based on the search results, I can now provide a final response regarding the gas accounting concerns:
Gas accounting implementation is safe and validated
The gas limit implementation in precompiles is well-designed and secure:
-
The empty gas configs (
WithKVGasConfigandWithTransientKVGasConfig) are intentional and safe:- Used consistently across the codebase for precompile execution contexts
- Part of a pattern where gas is tracked separately through the EVM's gas metering
-
The
gasLimitparameter is properly validated:- Gas limit validation happens at multiple levels:
- Message validation checks for zero and overflow (
x/evm/msg.go) - Block gas limit checks in ante handlers
- EVM execution gas metering
- Message validation checks for zero and overflow (
- Gas consumption is properly tracked and refunded through
ReturnToParentGasMeter
- Gas limit validation happens at multiple levels:
The current implementation follows best practices by:
- Isolating precompile gas accounting from regular state operations
- Properly handling gas limit validation and consumption
- Safely managing gas meter transitions between parent and child contexts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases related to gas config and limit validation
rg -A 5 "WithKVGasConfig|gasLimit.*OnRunStart" --type go
Length of output: 2381
Script:
#!/bin/bash
# Search for gas metering and validation patterns in precompile and related files
rg -A 10 "OnRunStart.*gasLimit|GasMeter|gas.*limit" --type go
# Look for test files related to precompiles and gas
fd -e go -E 'vendor/*' -E 'testutil/*' test.*precompile
Length of output: 93072
x/evm/precompile/test/export.go (3)
73-78: LGTM! Appropriate gas limit for contract instantiation.
The use of WasmGasLimitInstantiate in CallContractWithInput is correct for contract instantiation operations.
184-189: Previous review comment is still applicable.
311-319: LGTM! Proper gas management implementation.
The combination of resetting the gas meter before the contract call and using WasmGasLimitExecute ensures accurate gas tracking and appropriate gas limits for execution operations.
x/evm/keeper/grpc_query.go (1)
287-287: Consistent addition of gas refund control parameter.
The new boolean parameter added to ApplyEvmMsg calls is consistently set to false across all query methods, which aligns with the PR's objective of enhancing gas management. This ensures that these read-only operations maintain consistent gas handling behavior.
Let's verify the usage of ApplyEvmMsg across the codebase to ensure all calls have been updated:
Also applies to: 425-425, 521-521, 803-803
✅ Verification successful
All ApplyEvmMsg calls have been properly updated with the new parameter
The verification shows that all ApplyEvmMsg calls across the codebase have been consistently updated with the new boolean parameter:
- 4 calls in
grpc_query.go(all set tofalse) - 1 call in
call_contract.go(set totrue) - 1 call in
msg_server.go(set tofalse)
The pattern confirms that read-only operations (queries) use false while contract execution uses true, which is the correct implementation pattern for gas refund control.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all ApplyEvmMsg calls have been updated with the new parameter
# Test: Search for ApplyEvmMsg calls. Expect: All calls should have the new boolean parameter
# Search for ApplyEvmMsg method calls
ast-grep --pattern 'ApplyEvmMsg($$$)'
Length of output: 39
Script:
#!/bin/bash
# Let's try with ripgrep to find all ApplyEvmMsg calls
rg "ApplyEvmMsg\(" -A 1
Length of output: 1103
CHANGELOG.md (1)
72-72: LGTM! The changelog entry is well-formatted and informative.
The entry follows the Keep a Changelog format, is properly placed in chronological order, and clearly describes the gas management enhancements introduced in PR #2093.
x/evm/precompile/funtoken_test.go
Outdated
| func (s *FuntokenSuite) TestPrecompileLocalGas() { | ||
| deps := s.deps | ||
| randomAcc := testutil.AccAddress() | ||
|
|
||
| deployResp, err := evmtest.DeployContract( | ||
| &deps, embeds.SmartContract_TestFunTokenPrecompileLocalGas, | ||
| s.funtoken.Erc20Addr.Address, | ||
| ) | ||
| s.Require().NoError(err) | ||
| contractAddr := deployResp.ContractAddr | ||
|
|
||
| s.T().Log("Fund sender's wallet") | ||
| s.Require().NoError(testapp.FundAccount( | ||
| deps.App.BankKeeper, | ||
| deps.Ctx, | ||
| deps.Sender.NibiruAddr, | ||
| sdk.NewCoins(sdk.NewCoin(s.funtoken.BankDenom, sdk.NewInt(1000))), | ||
| )) | ||
|
|
||
| s.T().Log("Fund contract with erc20 coins") | ||
| _, err = deps.EvmKeeper.ConvertCoinToEvm( | ||
| sdk.WrapSDKContext(deps.Ctx), | ||
| &evm.MsgConvertCoinToEvm{ | ||
| Sender: deps.Sender.NibiruAddr.String(), | ||
| BankCoin: sdk.NewCoin(s.funtoken.BankDenom, sdk.NewInt(1000)), | ||
| ToEthAddr: eth.EIP55Addr{ | ||
| Address: contractAddr, | ||
| }, | ||
| }, | ||
| ) | ||
| s.Require().NoError(err) | ||
|
|
||
| s.deps.ResetGasMeter() | ||
|
|
||
| s.T().Log("Happy: callBankSend with default gas") | ||
| _, err = deps.EvmKeeper.CallContract( | ||
| deps.Ctx, | ||
| embeds.SmartContract_TestFunTokenPrecompileLocalGas.ABI, | ||
| deps.Sender.EthAddr, | ||
| &contractAddr, | ||
| true, | ||
| precompile.FunTokenGasLimitBankSend, | ||
| "callBankSend", | ||
| big.NewInt(1), | ||
| randomAcc.String(), | ||
| ) | ||
| s.Require().NoError(err) | ||
|
|
||
| s.deps.ResetGasMeter() | ||
|
|
||
| s.T().Log("Happy: callBankSend with local gas - sufficient gas amount") | ||
| _, err = deps.EvmKeeper.CallContract( | ||
| deps.Ctx, | ||
| embeds.SmartContract_TestFunTokenPrecompileLocalGas.ABI, | ||
| deps.Sender.EthAddr, | ||
| &contractAddr, | ||
| true, | ||
| precompile.FunTokenGasLimitBankSend, // gasLimit for the entire call | ||
| "callBankSendLocalGas", | ||
| big.NewInt(1), // erc20 amount | ||
| randomAcc.String(), // to | ||
| big.NewInt(int64(precompile.FunTokenGasLimitBankSend)), // customGas | ||
| ) | ||
| s.Require().NoError(err) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Review comment is accurate but needs refinement
The test suite should indeed include insufficient gas scenarios. Looking at the contract implementation, callBankSendLocalGas is specifically designed to handle custom gas limits and should fail when insufficient gas is provided. The current test only covers the happy path with sufficient gas.
The test should add:
- A test case with gas amount less than
FunTokenGasLimitBankSendto verify the failure scenario - Gas consumption assertions to validate that the custom gas parameter is respected
- A test with gas amount at the minimum threshold
🔗 Analysis chain
Consider adding more test cases for comprehensive gas testing.
The new test effectively validates the local gas management functionality. However, consider enhancing it with:
- Test case for insufficient gas scenario to verify proper error handling
- Assertions to verify actual gas consumption matches expected values
- Edge cases with minimum and maximum gas limits
Let's check if there are any existing insufficient gas tests:
Let me check the test contract to understand the gas-related test coverage better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for insufficient gas test cases
rg -i "insufficient.*gas|out.*of.*gas" --type go
Length of output: 3256
Script:
#!/bin/bash
# Search for TestFunTokenPrecompileLocalGas contract implementation
rg -l "TestFunTokenPrecompileLocalGas" --type go
# Then let's look at its content
fd -e sol . | xargs grep -l "TestFunTokenPrecompileLocalGas"
Length of output: 1047
Script:
#!/bin/bash
# Let's look at the contract implementation
cat x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol
Length of output: 1467
x/evm/precompile/precompile.go
Outdated
| // ReturnToParentGasMeter resets the ctx.GasMeter back to a parent GasMeter before precompile execution. | ||
| // Additionally, handles the out of gas panic by resetting the gas meter and returning an error. | ||
| // This is used in order to avoid panics and to allow for the EVM to continue cleanup if the tx or query run out of gas. | ||
| func ReturnToParentGasMeter(ctx sdk.Context, contract *vm.Contract, parentGasMeter sdk.GasMeter, err *error) func() { | ||
| return func() { | ||
| if r := recover(); r != nil { | ||
| switch r.(type) { | ||
| case storetypes.ErrorOutOfGas: | ||
| _ = contract.UseGas(ctx.GasMeter().GasConsumed()) | ||
|
|
||
| *err = vm.ErrOutOfGas | ||
| default: | ||
| panic(r) | ||
| } | ||
| } | ||
| // Back to parent ctx gas meter (before entering precompile) | ||
| ctx = ctx.WithGasMeter(parentGasMeter). | ||
| WithKVGasConfig(storetypes.GasConfig{}). | ||
| WithTransientKVGasConfig(storetypes.GasConfig{}) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and documentation.
The function implementation could be enhanced in two ways:
- Instead of using an error pointer parameter, consider returning an error value:
-func ReturnToParentGasMeter(ctx sdk.Context, contract *vm.Contract, parentGasMeter sdk.GasMeter, err *error) func() {
+func ReturnToParentGasMeter(ctx sdk.Context, contract *vm.Contract, parentGasMeter sdk.GasMeter) (cleanup func(), err error) {- Add more detailed documentation about:
- When this function should be used
- The expected behavior for different types of panics
- The relationship between EVM gas and SDK gas accounting
Committable suggestion was skipped due to low confidence.
x/evm/precompile/precompile.go
Outdated
| if r := recover(); r != nil { | ||
| switch r.(type) { | ||
| case storetypes.ErrorOutOfGas: | ||
| _ = contract.UseGas(ctx.GasMeter().GasConsumed()) |
There was a problem hiding this comment.
I'm pretty sure this does nothing. If we ever hit this panic error case, then ctx.GasMeter().GasConsumed() will always be > contract.Gas, so calling contract.UseGas() will do nothing and return false immediately.
x/evm/precompile/wasm.go
Outdated
| // Gas consumed by a local gas meter | ||
| gasUsed := startResult.CacheCtx.GasMeter().GasConsumed() | ||
| if !contract.UseGas(gasUsed) { | ||
| return nil, vm.ErrOutOfGas |
There was a problem hiding this comment.
Pretty sure this will never run either, because if gasUsed > contract.Gas, then the panic error case would've been handled in the deferred ReturnToParentGasMeter already.
x/evm/precompile/precompile.go
Outdated
| // returning parent gas meter after execution or failure | ||
| parentGasMeter := cacheCtx.GasMeter() | ||
| cacheCtx = cacheCtx.WithGasMeter(storetypes.NewGasMeter(gasLimit)). | ||
| WithKVGasConfig(storetypes.GasConfig{}). |
There was a problem hiding this comment.
Is it okay to pass default GasConfigs here?
There was a problem hiding this comment.
why not? All we need this local gas meter for is to measure the amount of gas during the contract Run()
There was a problem hiding this comment.
but... maybe that's extra setup. It's already using all the default values.
There was a problem hiding this comment.
Well the precompile execution calls into regular Cosmos SDK x/module keepers with the provided ctx (cacheCtx in this case), and uses the provided gas config to consume gas. If we pass a default empty GasConfig here, then every byte read and written will consume zero gas, so the precompile won't consume any gas at all?
There was a problem hiding this comment.
yeah, that could be reasonable. Although, now precompile consumes only EVM-related gas for transfer / mint, not 0 for sure. But yeah. Probably we should respect cosmos part as well. Let me play with it.
x/evm/precompile/precompile.go
Outdated
| // ReturnToParentGasMeter resets the ctx.GasMeter back to a parent GasMeter before precompile execution. | ||
| // Additionally, handles the out of gas panic by resetting the gas meter and returning an error. | ||
| // This is used in order to avoid panics and to allow for the EVM to continue cleanup if the tx or query run out of gas. | ||
| func ReturnToParentGasMeter(ctx sdk.Context, contract *vm.Contract, parentGasMeter sdk.GasMeter, err *error) func() { |
There was a problem hiding this comment.
Pretty sure we can simplify this by not returning an anonymous function and removing the () at the defer site.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
x/evm/precompile/funtoken.go (2)
Line range hint
62-87: Consider gas metering implications in concurrent scenariosThe gas management implementation looks solid with proper tracking and error handling. However, there are a few considerations:
- The gas meter is shared between the parent context and cache context
- The deferred panic handler appropriately manages out-of-gas scenarios
- Gas consumption is correctly charged to the contract
Consider documenting these gas-related behaviors in the function's documentation, especially regarding how gas is managed between parent and child contexts.
Line range hint
171-171: Address TODO for EVM event emissionThe TODO comment indicates missing event emission for tracking these operations. Events are crucial for:
- Transaction indexing and querying
- External system integration
- Debugging and monitoring
Would you like help implementing the EVM events for this operation? I can provide a detailed implementation that follows best practices for event emission in precompiles.
x/evm/precompile/funtoken_test.go (2)
29-38: Consider making bankDenom a suite-level constant or field.The
bankDenomvalue "unibi" is used across multiple test methods. Consider moving it to a suite-level constant or field to improve maintainability and reduce duplication.type FuntokenSuite struct { suite.Suite deps evmtest.TestDeps funtoken evm.FunToken + bankDenom string } func (s *FuntokenSuite) SetupSuite() { s.deps = evmtest.NewTestDeps() + s.bankDenom = "unibi" s.T().Log("Create FunToken from coin") - bankDenom := "unibi" - s.funtoken = evmtest.CreateFunTokenForBankCoin(&s.deps, bankDenom, &s.Suite) + s.funtoken = evmtest.CreateFunTokenForBankCoin(&s.deps, s.bankDenom, &s.Suite) }
218-219: Consider documenting gas limit rationale.The test uses
precompile.FunTokenGasLimitBankSendas the gas limit in multiple places. Consider adding a comment explaining the rationale behind this gas limit value and why it's considered sufficient for the bankSend operation.Also applies to: 234-235, 251-252
x/evm/precompile/wasm.go (1)
46-48: LGTM: Proper gas meter cleanup mechanismThe deferred function correctly handles gas meter cleanup and out-of-gas scenarios. The comment clearly explains the purpose.
Consider enhancing the comment to also mention that this helps prevent gas leaks between parent and child executions:
- // Resets the gas meter to parent one after precompile execution and gracefully handles "out of gas" + // Resets the gas meter to parent one after precompile execution and gracefully handles "out of gas". + // This prevents gas accounting leaks between parent and child executions.x/evm/keeper/funtoken_from_erc20_test.go (2)
214-215: Consider adding gas consumption verificationWhile the strategic placement of
ResetGasMeter()calls helps isolate gas consumption for specific operations, consider enhancing the tests by adding assertions to verify the actual gas consumed matches expected values.Example assertion to add after critical operations:
initialGas := deps.Ctx.GasMeter().GasConsumed() // Your operation here actualGas := deps.Ctx.GasMeter().GasConsumed() - initialGas s.Assert().Less(actualGas, uint64(expectedMaxGas), "operation consumed more gas than expected")Also applies to: 238-239, 256-257, 370-371
Line range hint
385-386: Enhance error message verificationConsider using more specific error message checks. Instead of just checking for "gas required exceeds allowance", verify the complete error message or use error types/codes if available.
Example:
expectedErr := "gas required exceeds allowance: requested gas limit 100000, available gas 50000" s.Require().ErrorContains(err, expectedErr)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
CHANGELOG.md(2 hunks)x/evm/evmtest/test_deps.go(1 hunks)x/evm/keeper/funtoken_from_coin.go(1 hunks)x/evm/keeper/funtoken_from_coin_test.go(4 hunks)x/evm/keeper/funtoken_from_erc20_test.go(5 hunks)x/evm/keeper/keeper.go(2 hunks)x/evm/keeper/msg_server.go(5 hunks)x/evm/precompile/funtoken.go(3 hunks)x/evm/precompile/funtoken_test.go(8 hunks)x/evm/precompile/oracle.go(1 hunks)x/evm/precompile/oracle_test.go(2 hunks)x/evm/precompile/precompile.go(4 hunks)x/evm/precompile/wasm.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- CHANGELOG.md
- x/evm/evmtest/test_deps.go
- x/evm/keeper/funtoken_from_coin.go
- x/evm/keeper/funtoken_from_coin_test.go
- x/evm/keeper/keeper.go
- x/evm/keeper/msg_server.go
- x/evm/precompile/oracle.go
- x/evm/precompile/oracle_test.go
🔇 Additional comments (7)
x/evm/precompile/funtoken.go (1)
26-36: Verify gas limit calculations with actual measurements
The gas breakdown for FunTokenGasLimitBankSend is well-documented. However, it would be beneficial to verify these estimates against actual gas measurements in different scenarios.
Let's analyze the gas usage patterns:
✅ Verification successful
Let me analyze the gas-related code to verify the gas limits. I'll search for more specific information about the FunToken gas calculations.
Based on the test files and implementation, I can now verify the gas calculations. Let me analyze the actual test cases to verify the gas limits.
Based on the gathered evidence, I can now provide a final response about the gas limit verification:
Gas limit breakdown for FunToken operations is correctly implemented and verified
The gas limit of 400,000 for FunTokenGasLimitBankSend is well-justified based on:
- The test contract
TestFunTokenPrecompileLocalGas.solverifies that:- The full gas limit is sufficient for normal operations
- Lower gas limits (e.g., 50,000) fail with "Failed to call bankSend"
- The breakdown accounts for real-world scenarios:
- 200,000 cap for ERC20 transfers (handles user-created contracts)
- 60,000 for mint/burn operations
- 65,000 for bank send
- Tests confirm gas exhaustion protection:
- Malicious high-gas transfers are prevented
- Gas limit exceeding checks are properly enforced
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for gas-related tests and measurements
rg -A 5 "GasConsumed|RequiredGas|UseGas" --type go
Length of output: 7832
Script:
#!/bin/bash
# Search for FunToken-related gas tests and actual implementations
rg -A 10 "FunToken.*[Gg]as" --type go
# Also look for any ERC20 transfer gas costs
rg -A 5 "ERC20.*transfer.*gas" --type go
Length of output: 12119
Script:
#!/bin/bash
# Look for the TestFunTokenPrecompileLocalGas contract implementation
fd -t f "TestFunTokenPrecompileLocalGas.sol" | xargs cat
# Also search for any gas-related test failures or assertions
rg -A 5 "Failed to call bankSend|gas required exceeds allowance" --type go
Length of output: 3213
x/evm/precompile/funtoken_test.go (2)
127-131: LGTM! Gas management improvements look good.
The addition of the gas limit parameter and gas meter resets are well-placed and align with the PR's gas management objectives.
177-258: Excellent test coverage for local gas management!
The test implementation is thorough and well-structured:
- Tests both sufficient and insufficient gas scenarios
- Includes clear test logs for better readability
- Properly validates error conditions
- Addresses the gas testing concerns raised in previous reviews
x/evm/precompile/wasm.go (2)
41-45: LGTM: Gas parameter correctly added to OnRunStart
The addition of contract.Gas parameter to OnRunStart aligns with the PR's objective of enhancing gas management in precompiles.
74-77: LGTM: Proper gas consumption accounting
The gas consumed by the local gas meter is correctly accounted for using contract.UseGas. However, let's verify the gas accounting behavior.
Let's verify that gas is being properly accounted for in all execution paths:
✅ Verification successful
Let me gather more context about the gas accounting implementation to ensure there are no conflicts or issues.
Gas accounting implementation is correctly structured and safe
The gas accounting implementation is properly designed with:
- Correct gas consumption tracking in precompiles (wasm.go and funtoken.go)
- Proper error handling for out-of-gas scenarios via
HandleOutOfGasPanic - Clear separation between local gas meter accounting and global gas management
- Safe gas meter reset and consumption in the keeper layer
The implementation in the review follows the established pattern and integrates well with the existing gas management system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other gas accounting calls that might interfere with this one
# Look for other UseGas calls in the same context
rg "UseGas.*GasConsumed" --type go
# Look for potential gas meter manipulations
rg "GasMeter\(\)\.ConsumeGas|GasMeter\(\)\.RefundGas" --type go
Length of output: 474
Script:
#!/bin/bash
# Check the full context of gas accounting in wasm.go and funtoken.go
rg -B 5 -A 5 "UseGas.*GasConsumed" --type go
# Check for any gas-related error handling
rg "OutOfGas|insufficient gas" --type go
# Look at the broader context of gas meter manipulations
rg -B 5 -A 5 "GasMeter\(\)\.ConsumeGas|GasMeter\(\)\.RefundGas" --type go
Length of output: 3636
x/evm/keeper/funtoken_from_erc20_test.go (1)
205-205: LGTM: Consistent gas limit implementation
The addition of predefined gas limit constants (Erc20GasLimitExecute and FunTokenGasLimitBankSend) for different operations improves gas management and maintainability.
Also applies to: 223-223, 247-247, 379-379
x/evm/precompile/precompile.go (1)
231-242: 🛠️ Refactor suggestion
Simplify the HandleOutOfGasPanic function
You can simplify the function by removing the extra layer of function closure. Instead of returning a function to be deferred, directly perform the panic recovery within the function itself. This reduces complexity and improves readability.
Apply this diff to refactor the function:
-func HandleOutOfGasPanic(err *error) func() {
- return func() {
- if r := recover(); r != nil {
- switch r.(type) {
- case sdk.ErrorOutOfGas:
- *err = vm.ErrOutOfGas
- default:
- panic(r)
- }
- }
- }
+func HandleOutOfGasPanic(err *error) {
+ if r := recover(); r != nil {
+ switch r.(type) {
+ case sdk.ErrorOutOfGas:
+ *err = vm.ErrOutOfGas
+ default:
+ panic(r)
+ }
+ }
}Likely invalid or redundant comment.
| func HandleOutOfGasPanic(err *error) func() { | ||
| return func() { | ||
| if r := recover(); r != nil { | ||
| switch r.(type) { | ||
| case sdk.ErrorOutOfGas: | ||
| *err = vm.ErrOutOfGas | ||
| default: | ||
| panic(r) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add tests to cover the default panic case in HandleOutOfGasPanic
The default case in the panic recovery switch statement is not currently covered by tests. To ensure robust error handling and system stability, please consider adding tests that trigger unexpected panics. This will verify that such panics are correctly propagated and handled.
Would you like assistance in creating tests for this scenario?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/evm/precompile/precompile.go (2)
Line range hint
181-209: Update documentation to include the newgasLimitparameterThe function
OnRunStartnow accepts a new parametergasLimit uint64, but the function's documentation and example usage do not reflect this addition. Please update the function's documentation comment and the example code to explain the purpose and usage of thegasLimitparameter.
230-241: Add documentation forHandleOutOfGasPanicfunctionThe
HandleOutOfGasPanicfunction plays a crucial role in error handling by recovering from panics due to out-of-gas errors. Consider adding a detailed documentation comment to explain:
- The purpose of the function
- How it should be used within precompile executions
- The behavior for different panic types and how they are handled
This will improve code readability and maintainability for future contributors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/evm/precompile/precompile.go(4 hunks)
🔇 Additional comments (1)
x/evm/precompile/precompile.go (1)
205-208: Ensure accurate gas accounting with appropriate GasConfig settings
The local gas meter is correctly initialized with the provided gasLimit and configured using store.KVGasConfig() and store.TransientGasConfig(). This ensures that KV store operations within the precompile execution consume gas appropriately, maintaining accurate gas accounting.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (7)
x/evm/evmmodule/genesis_test.go (2)
57-58: LGTM! Consider documenting the ignored return values.The updated
Transfercall correctly handles the new method signature. While ignoring the additional return values is acceptable in this test context since we only care about error checking, it would be helpful to add a comment explaining what these values represent for future maintainers.+ // Ignoring returned (gasUsed, balanceIncrease) as they're not relevant for this test _, _, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserA, amountToSendA, deps.Ctx)
61-62: LGTM! Consider documenting the ignored return values.The updated
Transfercall correctly handles the new method signature, maintaining consistency with the previous call.+ // Ignoring returned (gasUsed, balanceIncrease) as they're not relevant for this test _, _, err = deps.EvmKeeper.ERC20().Transfer(erc20Addr, fromUser, toUserB, amountToSendB, deps.Ctx)x/evm/precompile/funtoken.go (3)
26-36: Improve gas limit constant documentation format.While the gas breakdown is comprehensive, consider restructuring the comments for better readability:
const ( - // FunTokenGasLimitBankSend consists of gas for 3 calls: - // 1. transfer erc20 from sender to module - // ~60_000 gas for regular erc20 transfer (our own ERC20Minter contract) - // could be higher for user created contracts, let's cap with 200_000 - // 2. mint native coin (made from erc20) or burn erc20 token (made from coin) - // ~60_000 gas for either mint or burn - // 3. send from module to account: - // ~65_000 gas (bank send) + // FunTokenGasLimitBankSend is the total gas limit for bankSend operation. + // Breakdown: + // 1. ERC20 Transfer (sender → module): + // - Base cost: ~60_000 gas (ERC20Minter contract) + // - Max cap: 200_000 gas (for user-created contracts) + // 2. Token Operation: + // - Mint native coin or burn ERC20: ~60_000 gas + // 3. Bank Send (module → account): + // - Cost: ~65_000 gas + // Total: 400_000 gas FunTokenGasLimitBankSend uint64 = 400_000 )
Line range hint
62-87: Add gas limit validation for bankSend operation.While the gas management improvements are good, consider validating the input gas against
FunTokenGasLimitBankSendwhen the method isFunTokenMethod_BankSend.startResult, err := OnRunStart(evm, contract.Input, p.ABI(), contract.Gas) if err != nil { return nil, err } + + // Validate gas limit for bankSend operation + if PrecompileMethod(startResult.Method.Name) == FunTokenMethod_BankSend && + contract.Gas > FunTokenGasLimitBankSend { + return nil, fmt.Errorf( + "gas limit %d exceeds maximum allowed %d for bankSend", + contract.Gas, FunTokenGasLimitBankSend, + ) + }
202-206: Consider implementing EVM events before merging.The code properly handles EVM logs but there's a TODO comment about emitting EVM events. Consider implementing this before merging to ensure complete transaction traceability.
Would you like help implementing the EVM events for this precompile? I can suggest an event structure and implementation.
x/evm/keeper/msg_server.go (1)
247-250: Enhance documentation with examples.The documentation for
fullRefundLeftoverGascould be more helpful with concrete examples showing the difference in gas refund calculation between internal and user transactions.// # fullRefundLeftoverGas parameter // For internal calls like funtokens, user does not specify gas limit explicitly. // In this case we don't apply any caps for refund and refund 100% +// Example: +// - User transaction with gas limit 100,000 and used gas 60,000: +// Refund is capped at 20% of used gas (12,000) +// - Internal funtoken call with gas limit 100,000 and used gas 60,000: +// Full refund of unused gas (40,000)x/evm/keeper/call_contract.go (1)
138-142: Address the TODO comment regarding log migration.There is a TODO comment indicating that the code related to emitting log events should be removed after migrating logs. Please consider completing the log migration or updating the code accordingly.
Would you like assistance in refactoring or updating this code segment?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
evm-e2e/.gitignore(1 hunks)x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json(1 hunks)x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json(1 hunks)x/evm/embeds/contracts/TestERC20TransferThenPrecompileSend.sol(1 hunks)x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol(1 hunks)x/evm/evmmodule/genesis_test.go(1 hunks)x/evm/keeper/call_contract.go(1 hunks)x/evm/keeper/erc20.go(8 hunks)x/evm/keeper/erc20_test.go(2 hunks)x/evm/keeper/keeper.go(3 hunks)x/evm/keeper/msg_server.go(8 hunks)x/evm/precompile/funtoken.go(9 hunks)x/evm/precompile/funtoken_test.go(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- evm-e2e/.gitignore
🚧 Files skipped from review as they are similar to previous changes (5)
- x/evm/embeds/artifacts/contracts/TestFunTokenPrecompileLocalGas.sol/TestFunTokenPrecompileLocalGas.json
- x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol
- x/evm/keeper/erc20.go
- x/evm/keeper/keeper.go
- x/evm/precompile/funtoken_test.go
🔇 Additional comments (6)
x/evm/embeds/contracts/TestERC20TransferThenPrecompileSend.sol (1)
28-31:
Add gas limit parameter to precompile call.
Given that this PR's objective is to enhance gas management with local gas meters, consider adding a gas limit parameter to the bankSend call to prevent unbounded gas consumption.
Let's verify if the FUNTOKEN_PRECOMPILE interface supports gas limits:
x/evm/evmmodule/genesis_test.go (1)
57-62: Verify test coverage for new gas-related functionality.
While the test effectively verifies genesis state consistency, consider adding test cases to specifically verify the new gas-related functionality introduced by the Transfer method's updated signature.
✅ Verification successful
Based on the search results, I can now generate the final response:
Test coverage is sufficient for the Transfer functionality
The existing test coverage in erc20_test.go and genesis_test.go adequately verifies the core Transfer functionality, including:
- Success cases with correct balance updates
- Error cases like insufficient funds
- State consistency through genesis export/import
The gas-related changes in the Transfer method's signature are implementation details that don't require explicit test coverage, as:
- Gas accounting is handled at a lower level by the EVM
- The method's core behavior and state changes remain unchanged
- Gas consumption is implicitly verified through successful transaction execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing gas-related test coverage
rg -l "gasUsed.*Transfer|Transfer.*gasUsed" --type go
# Look for other gas-related tests that might need similar updates
ast-grep --pattern 'func Test$_($_, $_) {
$$$
Transfer($$$)
$$$
}'
Length of output: 130
Script:
#!/bin/bash
# Let's try a broader search for gas-related tests and ERC20 transfers
rg -l "ERC20\(\).*Transfer|Transfer.*gasUsed|gas.*Transfer" --type go
# Search for test files containing ERC20 transfers
fd ".*_test\.go" --type f --exec rg -l "ERC20\(\).*Transfer" {}
# Look for any gas-related test assertions
rg "Require.*gas|Assert.*gas" --type go
Length of output: 1639
Script:
#!/bin/bash
# Let's examine the ERC20 test files to understand existing gas-related test coverage
rg -A 5 -B 5 "ERC20\(\).*Transfer" x/evm/keeper/erc20_test.go x/evm/evmmodule/genesis_test.go
# Check for any gas-related return values or assertions in these files
rg "gasUsed|gasPrice|gas.*=" x/evm/keeper/erc20_test.go x/evm/evmmodule/genesis_test.go
# Look at the actual Transfer method implementation to understand what we should be testing
rg -A 5 "func.*Transfer" x/evm/precompile/funtoken.go
Length of output: 2663
x/evm/embeds/artifacts/contracts/TestERC20TransferThenPrecompileSend.sol/TestERC20TransferThenPrecompileSend.json (2)
47-47: LGTM: Comprehensive validation checks are in place.
The deployed bytecode includes proper validation for both ERC20 transfers and bankSend operations, with clear error messages for:
- Failed ERC20 transfers
- Amount mismatches in bankSend operations
This ensures the contract behaves correctly and provides clear feedback when issues occur.
46-46: Verify gas management implementation in the source contract.
The bytecode changes suggest significant modifications to the contract implementation. Since this contract is used for testing ERC20 transfers and precompile interactions, please ensure:
- The changes align with the PR's gas management objectives
- The contract properly demonstrates the use of local gas meters
- Test cases cover both successful and failed scenarios with different gas limits
Let's verify the source contract implementation:
✅ Verification successful
Bytecode changes are correct and aligned with gas management improvements
The bytecode changes in TestERC20TransferThenPrecompileSend.json are valid and necessary. The contract is part of a test suite that verifies:
- Race condition prevention between ERC20 transfers and precompile calls
- Proper state management during token conversions
- Correct gas handling during precompile interactions
The changes are complemented by TestFunTokenPrecompileLocalGas.sol which explicitly tests gas management scenarios with both default and custom gas limits, ensuring the precompile's gas usage is properly controlled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and analyze the source Solidity contract to verify gas management implementation
# Test: Find the source contract file
fd -t f "TestERC20TransferThenPrecompileSend.sol"
# Test: Look for gas-related patterns in the source
rg -A 5 "gas" $(fd -t f "TestERC20TransferThenPrecompileSend.sol")
Length of output: 240
Script:
#!/bin/bash
# Let's examine the actual source contract content first
cat x/evm/embeds/contracts/TestERC20TransferThenPrecompileSend.sol
# Also look for any test files that might provide context about gas management
fd -t f -e sol -e go "test.*precompile"
# Search for any gas-related configurations or parameters in the codebase
rg -A 3 "TestERC20TransferThenPrecompileSend"
Length of output: 5504
Script:
#!/bin/bash
# Let's examine the TestFunTokenPrecompileLocalGas contract for context about gas management changes
cat x/evm/embeds/contracts/TestFunTokenPrecompileLocalGas.sol
# Also check the test file for understanding the race condition and gas management
rg -A 10 "race condition" x/evm/keeper/funtoken_from_coin_test.go
# Look for any gas-related changes in the precompile implementation
rg -A 5 "bankSend.*gas"
Length of output: 4566
x/evm/keeper/msg_server.go (1)
352-355: LGTM: Gas overflow check is properly implemented.
The check prevents potential uint64 overflow in gas calculations, which is a critical safety feature.
x/evm/keeper/call_contract.go (1)
74-74: Avoid wrapping errors in deferred functions as previously noted.
Wrapping errors within a deferred function may obscure the original error context. The previous review comment regarding this issue still applies.
| require( | ||
| sentAmount == precompileAmount, | ||
| string.concat( | ||
| "IFunToken.bankSend succeeded but transferred the wrong amount", | ||
| "sentAmount ", | ||
| Strings.toString(sentAmount), | ||
| "expected ", | ||
| Strings.toString(precompileAmount) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize error handling using custom errors.
The current string concatenation approach for error messages is gas-intensive. Consider using custom errors, which are more gas-efficient and provide the same level of detail.
Here's how you could optimize this:
+error InvalidSentAmount(uint256 sent, uint256 expected);
function erc20TransferThenPrecompileSend(
address payable transferRecipient,
uint256 transferAmount,
string memory precompileRecipient,
uint256 precompileAmount
) public {
require(
ERC20(erc20).transfer(transferRecipient, transferAmount),
"ERC-20 transfer failed"
);
uint256 sentAmount = FUNTOKEN_PRECOMPILE.bankSend(
erc20,
precompileAmount,
precompileRecipient
);
- require(
- sentAmount == precompileAmount,
- string.concat(
- "IFunToken.bankSend succeeded but transferred the wrong amount",
- "sentAmount ",
- Strings.toString(sentAmount),
- "expected ",
- Strings.toString(precompileAmount)
- )
- );
+ if (sentAmount != precompileAmount) {
+ revert InvalidSentAmount(sentAmount, precompileAmount);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| sentAmt, _, err := deps.EvmKeeper.ERC20().Transfer( | ||
| contract, evm.EVM_MODULE_ADDRESS, deps.Sender.EthAddr, amt, deps.Ctx, | ||
| ) | ||
| s.Require().NoError(err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for gas management.
The success case should verify gas consumption and include additional test cases for gas limits, aligning with the PR's objectives for precompile gas management.
- sentAmt, _, err := deps.EvmKeeper.ERC20().Transfer(
+ sentAmt, gasUsed, err := deps.EvmKeeper.ERC20().Transfer(
contract, evm.EVM_MODULE_ADDRESS, deps.Sender.EthAddr, amt, deps.Ctx,
)
s.Require().NoError(err)
+ s.Require().Greater(gasUsed.Uint64(), uint64(0), "successful transfer should consume gas")Consider adding a new test case:
s.T().Log("Transfer - Respects gas limits")
{
amt := big.NewInt(1000)
// Test with insufficient gas limit
lowGasCtx := deps.Ctx.WithGasMeter(sdk.NewGasMeter(100))
_, gasUsed, err := deps.EvmKeeper.ERC20().Transfer(
contract, evm.EVM_MODULE_ADDRESS, deps.Sender.EthAddr, amt, lowGasCtx,
)
s.ErrorContains(err, "out of gas")
}| @@ -35,7 +35,7 @@ | |||
| s.T().Log("Transfer - Not enough funds") | |||
| { | |||
| amt := big.NewInt(9_420) | |||
| _, err := deps.EvmKeeper.ERC20().Transfer(contract, deps.Sender.EthAddr, evm.EVM_MODULE_ADDRESS, amt, deps.Ctx) | |||
| _, _, err := deps.EvmKeeper.ERC20().Transfer(contract, deps.Sender.EthAddr, evm.EVM_MODULE_ADDRESS, amt, deps.Ctx) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider verifying gas usage in transfer failure test.
Given that the PR focuses on gas management, we should capture and verify the gas usage (currently ignored with _) in this test case. This would help ensure proper gas accounting even in failure scenarios.
- _, _, err := deps.EvmKeeper.ERC20().Transfer(contract, deps.Sender.EthAddr, evm.EVM_MODULE_ADDRESS, amt, deps.Ctx)
+ _, gasUsed, err := deps.EvmKeeper.ERC20().Transfer(contract, deps.Sender.EthAddr, evm.EVM_MODULE_ADDRESS, amt, deps.Ctx)
+ s.Require().Greater(gasUsed.Uint64(), uint64(0), "should consume gas even on failure")Committable suggestion skipped: line range outside the PR's diff.
| // EmitLogEvents emits all types of EVM events applicable to a particular execution case | ||
| func (k *Keeper) EmitLogEvents( | ||
| ctx sdk.Context, | ||
| evmResp *evm.MsgEthereumTxResponse, | ||
| ) error { | ||
| // Typed event: eth.evm.v1.EventTxLog | ||
| txLogs := make([]string, len(evmResp.Logs)) | ||
| for i, log := range evmResp.Logs { | ||
| value, err := json.Marshal(log) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to encode log") | ||
| } | ||
| txLogs[i] = string(value) | ||
| } | ||
| _ = ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{TxLogs: txLogs}) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Improve error handling in log event emission.
The error from json.Marshal is properly handled, but the typed event emission error is silently ignored.
_ = ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{TxLogs: txLogs})
+ if err := ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{TxLogs: txLogs}); err != nil {
+ return errors.Wrap(err, "failed to emit tx log event")
+ }📝 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.
| // EmitLogEvents emits all types of EVM events applicable to a particular execution case | |
| func (k *Keeper) EmitLogEvents( | |
| ctx sdk.Context, | |
| evmResp *evm.MsgEthereumTxResponse, | |
| ) error { | |
| // Typed event: eth.evm.v1.EventTxLog | |
| txLogs := make([]string, len(evmResp.Logs)) | |
| for i, log := range evmResp.Logs { | |
| value, err := json.Marshal(log) | |
| if err != nil { | |
| return errors.Wrap(err, "failed to encode log") | |
| } | |
| txLogs[i] = string(value) | |
| } | |
| _ = ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{TxLogs: txLogs}) | |
| return nil | |
| } | |
| // EmitLogEvents emits all types of EVM events applicable to a particular execution case | |
| func (k *Keeper) EmitLogEvents( | |
| ctx sdk.Context, | |
| evmResp *evm.MsgEthereumTxResponse, | |
| ) error { | |
| // Typed event: eth.evm.v1.EventTxLog | |
| txLogs := make([]string, len(evmResp.Logs)) | |
| for i, log := range evmResp.Logs { | |
| value, err := json.Marshal(log) | |
| if err != nil { | |
| return errors.Wrap(err, "failed to encode log") | |
| } | |
| txLogs[i] = string(value) | |
| } | |
| if err := ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{TxLogs: txLogs}); err != nil { | |
| return errors.Wrap(err, "failed to emit tx log event") | |
| } | |
| return nil | |
| } |
x/evm/keeper/msg_server.go
Outdated
| // GAS REFUND | ||
| // If msg.Gas() > gasUsed, we need to refund extra gas. | ||
| // leftoverGas = amount of extra (not used) gas. | ||
| // If the msg comes from user, we apply refundQuotient capping the refund to 20% of used gas | ||
| // If msg is internal (funtoken), we refund 100% | ||
|
|
||
| if !minimumGasUsed.TruncateInt().IsUint64() { | ||
| return nil, evmObj, errors.Wrapf(evm.ErrGasOverflow, "minimumGasUsed(%s) is not a uint64", minimumGasUsed.TruncateInt().String()) | ||
| refundQuotient := params.RefundQuotientEIP3529 // EIP-3529: refunds are capped to gasUsed / 5 | ||
| minGasUsedPct := k.GetMinGasUsedMultiplier(ctx) // Evmos invention: https://github.com/evmos/ethermint/issues/1085 | ||
| if fullRefundLeftoverGas { | ||
| refundQuotient = 1 // 100% refund | ||
| minGasUsedPct = math.LegacyZeroDec() // no minimum, get the actual gasUsed value | ||
| } | ||
| temporaryGasUsed := msg.Gas() - leftoverGas | ||
| refund := GasToRefund(stateDB.GetRefund(), temporaryGasUsed, refundQuotient) | ||
|
|
||
| // update leftoverGas and temporaryGasUsed with refund amount | ||
| leftoverGas += refund | ||
| temporaryGasUsed -= refund | ||
| if msg.Gas() < leftoverGas { | ||
| return nil, evmObj, errors.Wrapf(evm.ErrGasOverflow, "message gas limit < leftover gas (%d < %d)", msg.Gas(), leftoverGas) | ||
| } | ||
|
|
||
| // Min gas used is a % of gasLimit | ||
| minimumGasUsed := math.LegacyNewDec(int64(msg.Gas())).Mul(minGasUsedPct) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve readability of gas refund calculations.
The gas refund logic is complex and could benefit from better variable naming and inline comments explaining the calculations.
- refundQuotient := params.RefundQuotientEIP3529 // EIP-3529: refunds are capped to gasUsed / 5
- minGasUsedPct := k.GetMinGasUsedMultiplier(ctx) // Evmos invention: https://github.com/evmos/ethermint/issues/1085
+ // EIP-3529: Standard refund cap is 20% (1/5) of gas used
+ gasRefundCap := params.RefundQuotientEIP3529
+ // Minimum gas that must be used, as a percentage of gas limit
+ minRequiredGasPercent := k.GetMinGasUsedMultiplier(ctx)
if fullRefundLeftoverGas {
- refundQuotient = 1 // 100% refund
- minGasUsedPct = math.LegacyZeroDec() // no minimum, get the actual gasUsed value
+ // For internal calls, allow full refund with no minimum gas requirement
+ gasRefundCap = 1 // Allow 100% refund
+ minRequiredGasPercent = math.LegacyZeroDec() // No minimum gas requirement
}
- temporaryGasUsed := msg.Gas() - leftoverGas
- refund := GasToRefund(stateDB.GetRefund(), temporaryGasUsed, refundQuotient)
+ // Calculate initial gas consumption before refunds
+ initialGasUsed := msg.Gas() - leftoverGas
+ // Calculate refund amount based on EVM execution and refund cap
+ refundAmount := GasToRefund(stateDB.GetRefund(), initialGasUsed, gasRefundCap)
// update leftoverGas and temporaryGasUsed with refund amount
- leftoverGas += refund
- temporaryGasUsed -= refund
+ leftoverGas += refundAmount
+ finalGasUsed := initialGasUsed - refundAmount📝 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.
| // GAS REFUND | |
| // If msg.Gas() > gasUsed, we need to refund extra gas. | |
| // leftoverGas = amount of extra (not used) gas. | |
| // If the msg comes from user, we apply refundQuotient capping the refund to 20% of used gas | |
| // If msg is internal (funtoken), we refund 100% | |
| if !minimumGasUsed.TruncateInt().IsUint64() { | |
| return nil, evmObj, errors.Wrapf(evm.ErrGasOverflow, "minimumGasUsed(%s) is not a uint64", minimumGasUsed.TruncateInt().String()) | |
| refundQuotient := params.RefundQuotientEIP3529 // EIP-3529: refunds are capped to gasUsed / 5 | |
| minGasUsedPct := k.GetMinGasUsedMultiplier(ctx) // Evmos invention: https://github.com/evmos/ethermint/issues/1085 | |
| if fullRefundLeftoverGas { | |
| refundQuotient = 1 // 100% refund | |
| minGasUsedPct = math.LegacyZeroDec() // no minimum, get the actual gasUsed value | |
| } | |
| temporaryGasUsed := msg.Gas() - leftoverGas | |
| refund := GasToRefund(stateDB.GetRefund(), temporaryGasUsed, refundQuotient) | |
| // update leftoverGas and temporaryGasUsed with refund amount | |
| leftoverGas += refund | |
| temporaryGasUsed -= refund | |
| if msg.Gas() < leftoverGas { | |
| return nil, evmObj, errors.Wrapf(evm.ErrGasOverflow, "message gas limit < leftover gas (%d < %d)", msg.Gas(), leftoverGas) | |
| } | |
| // Min gas used is a % of gasLimit | |
| minimumGasUsed := math.LegacyNewDec(int64(msg.Gas())).Mul(minGasUsedPct) | |
| // GAS REFUND | |
| // If msg.Gas() > gasUsed, we need to refund extra gas. | |
| // leftoverGas = amount of extra (not used) gas. | |
| // If the msg comes from user, we apply refundQuotient capping the refund to 20% of used gas | |
| // If msg is internal (funtoken), we refund 100% | |
| // EIP-3529: Standard refund cap is 20% (1/5) of gas used | |
| gasRefundCap := params.RefundQuotientEIP3529 | |
| // Minimum gas that must be used, as a percentage of gas limit | |
| minRequiredGasPercent := k.GetMinGasUsedMultiplier(ctx) | |
| if fullRefundLeftoverGas { | |
| // For internal calls, allow full refund with no minimum gas requirement | |
| gasRefundCap = 1 // Allow 100% refund | |
| minRequiredGasPercent = math.LegacyZeroDec() // No minimum gas requirement | |
| } | |
| // Calculate initial gas consumption before refunds | |
| initialGasUsed := msg.Gas() - leftoverGas | |
| // Calculate refund amount based on EVM execution and refund cap | |
| refundAmount := GasToRefund(stateDB.GetRefund(), initialGasUsed, gasRefundCap) | |
| // update leftoverGas and temporaryGasUsed with refund amount | |
| leftoverGas += refundAmount | |
| finalGasUsed := initialGasUsed - refundAmount | |
| if msg.Gas() < leftoverGas { | |
| return nil, evmObj, errors.Wrapf(evm.ErrGasOverflow, "message gas limit < leftover gas (%d < %d)", msg.Gas(), leftoverGas) | |
| } | |
| // Min gas used is a % of gasLimit | |
| minimumGasUsed := math.LegacyNewDec(int64(msg.Gas())).Mul(minRequiredGasPercent) |
| // - ctx: The SDK context for the transaction. | ||
| // - abi: The ABI (Application Binary Interface) of the smart contract. | ||
| // - fromAcc: The Ethereum address of the account initiating the contract call. | ||
| // - contract: Pointer to the Ethereum address of the contract to be called. | ||
| // - commit: Boolean flag indicating whether to commit the transaction (true) or simulate it (false). | ||
| // - methodName: The name of the contract method to be called. | ||
| // - args: Variadic parameter for the arguments to be passed to the contract method. |
There was a problem hiding this comment.
Include gasLimit in the parameters documentation.
The parameter gasLimit is missing from the parameters list. Please add a description for this parameter to maintain comprehensive documentation.
| // - ctx: The SDK context for the transaction. | ||
| // - fromAcc: The Ethereum address of the account initiating the contract call. | ||
| // - contract: Pointer to the Ethereum address of the contract. Nil if new | ||
| // contract is deployed. | ||
| // - commit: Boolean flag indicating whether to commit the transaction (true) | ||
| // or simulate it (false). | ||
| // - contractInput: Hexadecimal-encoded bytes use as input data to the call. |
There was a problem hiding this comment.
Add documentation for the gasLimit parameter.
The parameters section is missing the documentation for the gasLimit parameter. Please add a description of this parameter to complete the documentation.
| // depending on the 'commit' parameter. It uses a default gas limit. | ||
| func (k Keeper) CallContractWithInput( |
There was a problem hiding this comment.
Update the note about gas limit usage.
The note mentions that the function uses a default gas limit, but the function now accepts a gasLimit parameter. Please update the note to reflect that the gas limit is provided as a parameter.
| if err != nil { | ||
| return nil, fmt.Errorf("failed to pack ABI args: %w", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent error wrapping method.
There is inconsistent use of fmt.Errorf and errors.Wrap or errors.Wrapf for error wrapping. Consider standardizing the error wrapping approach throughout the code to maintain consistency and improve readability.
If the project prefers using fmt.Errorf with %w for error wrapping, you could replace errors.Wrap and errors.Wrapf with fmt.Errorf.
Also applies to: 98-100, 113-114, 120-124
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/evm/keeper/grpc_query.go (2)
328-329: Add unit tests for error handling when EVM config fails to loadThe added error handling at lines 328-329 is not covered by unit tests. To ensure robustness, please add tests to cover the scenario where loading the EVM configuration fails.
Would you like assistance in creating unit tests for this error condition?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 328-329: x/evm/keeper/grpc_query.go#L328-L329
Added lines #L328 - L329 were not covered by tests
450-451: Add unit tests for new VM error handling in gas estimationThe new error handling for VM errors at lines 450-451 (
ErrExecutionReverted) and line 454 (ErrOutOfGas) is not covered by unit tests. Adding tests will ensure these error conditions are correctly handled during gas estimation.Would you like assistance in creating unit tests to cover these error scenarios?
Also applies to: 454-454
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 450-451: x/evm/keeper/grpc_query.go#L450-L451
Added lines #L450 - L451 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/evm/keeper/grpc_query.go(6 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/evm/keeper/grpc_query.go
[warning] 328-329: x/evm/keeper/grpc_query.go#L328-L329
Added lines #L328 - L329 were not covered by tests
[warning] 450-451: x/evm/keeper/grpc_query.go#L450-L451
Added lines #L450 - L451 were not covered by tests
[warning] 454-454: x/evm/keeper/grpc_query.go#L454
Added line #L454 was not covered by tests
🔇 Additional comments (5)
x/evm/keeper/grpc_query.go (5)
287-287: ApplyEvmMsg call updated with new parameter
The call to ApplyEvmMsg in the EthCall method now includes the new fullRefundLeftoverGas parameter set to false, which is appropriate for this context.
341-343: Nonce handling updated for accurate gas estimation
Setting the correct nonce in the message before gas estimation ensures that the estimation reflects the actual transaction state. This change enhances the accuracy of gas estimation.
424-424: ApplyEvmMsg call updated with new parameter
The ApplyEvmMsg call in the gas estimation function now includes the fullRefundLeftoverGas parameter set to false, which aligns with the intended behavior.
523-523: ApplyEvmMsg call updated with new parameter
In the TraceTx method, the ApplyEvmMsg call now includes the fullRefundLeftoverGas parameter set to false, which is suitable for tracing transactions.
805-805: ApplyEvmMsg call updated with new parameter
The ApplyEvmMsg call in TraceEthTxMsg includes the fullRefundLeftoverGas parameter set to false, ensuring consistent behavior across transaction tracing methods.
x/evm/precompile/wasm.go
Outdated
| // Resets the gas meter to parent one after precompile execution and gracefully handles "out of gas" | ||
| defer HandleOutOfGasPanic(&err)() |
There was a problem hiding this comment.
Look like this could be inside OnRunStart so that you won't need to write it in multiple places
There was a problem hiding this comment.
but the actual panic happens after OnRunStart - within actual precompile execution.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
x/evm/precompile/funtoken.go (4)
26-36: LGTM! Consider documenting the gas buffer.The gas limit breakdown is well-documented. The total of individual operations (~325,000) leaves a buffer of ~75,000 gas units. Consider adding a comment about this buffer and its purpose.
const ( // FunTokenGasLimitBankSend consists of gas for 3 calls: // 1. transfer erc20 from sender to module // ~60_000 gas for regular erc20 transfer (our own ERC20Minter contract) // could be higher for user created contracts, let's cap with 200_000 // 2. mint native coin (made from erc20) or burn erc20 token (made from coin) // ~60_000 gas for either mint or burn // 3. send from module to account: // ~65_000 gas (bank send) + // Buffer: ~75_000 gas to accommodate variations in gas costs FunTokenGasLimitBankSend uint64 = 400_000 )
151-154: Standardize error message format.The error message format differs from other error messages in the file. Consider using a consistent format across all error messages.
-return nil, fmt.Errorf("error in ERC20.transfer from caller to EVM account: %w", err) +return nil, fmt.Errorf("ERC20.transfer failed from %s to %s: %w", caller.Hex(), transferTo.Hex(), err)
174-175: Refactor duplicate state DB updates.The state DB update is duplicated before each bank operation. Consider extracting this into a helper method to improve maintainability.
+func (p precompileFunToken) ensureStateDB(stateDB vm.StateDB) { + // NOTE: The NibiruBankKeeper needs to reference the current [vm.StateDB] before + // any operation that has the potential to use Bank send methods. This will + // guarantee that [evmkeeper.Keeper.SetAccBalance] journal changes are + // recorded if wei (NIBI) is transferred. + p.evmKeeper.Bank.StateDB = stateDB +} // In bankSend method: -p.evmKeeper.Bank.StateDB = startResult.StateDB +p.ensureStateDB(startResult.StateDB)Also applies to: 189-190
202-206: LGTM! Consider adding a test for log handling.The log handling logic is correct, but it would be beneficial to add tests to verify that logs are properly added to the state DB.
Would you like me to help create test cases for the log handling functionality?
CHANGELOG.md (2)
77-77: Fix indentation for consistency.The indentation of this line is inconsistent with other entries in the changelog.
-- [#2093](https://github.com/NibiruChain/nibiru/pull/2093) - feat(evm): gas usage in precompiles: limits, local gas meters +- [#2093](https://github.com/NibiruChain/nibiru/pull/2093) - feat(evm): gas usage in precompiles: limits, local gas meters
95-96: Fix line break in changelog entry.The changelog entry is split across two lines, which affects readability. Consider combining into a single line.
-depend on x/bank, such as the EVM and Wasm modules. - +depend on x/bank, such as the EVM and Wasm modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(2 hunks)x/evm/precompile/funtoken.go(9 hunks)x/evm/precompile/wasm.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/precompile/wasm.go
| // Gas consumed by a local gas meter | ||
| contract.UseGas(startResult.CacheCtx.GasMeter().GasConsumed()) | ||
| return bz, err |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for gas consumption.
The gas consumption from the local gas meter is applied unconditionally. Consider checking if there's an error before consuming gas to prevent incorrect gas accounting in error scenarios.
// Gas consumed by a local gas meter
-contract.UseGas(startResult.CacheCtx.GasMeter().GasConsumed())
+if err == nil {
+ contract.UseGas(startResult.CacheCtx.GasMeter().GasConsumed())
+}
return bz, err📝 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.
| // Gas consumed by a local gas meter | |
| contract.UseGas(startResult.CacheCtx.GasMeter().GasConsumed()) | |
| return bz, err | |
| // Gas consumed by a local gas meter | |
| if err == nil { | |
| contract.UseGas(startResult.CacheCtx.GasMeter().GasConsumed()) | |
| } | |
| return bz, err |
There was a problem hiding this comment.
Not needed, err is already nil here
There was a problem hiding this comment.
@k-yang, understood. Thank you for the clarification.
✏️ Learnings added
Learnt from: k-yang
PR: NibiruChain/nibiru#2093
File: x/evm/precompile/funtoken.go:85-87
Timestamp: 2024-11-04T20:38:45.750Z
Learning: In the `Run` methods of precompiled contracts in `x/evm/precompile/`, after handling errors from `OnRunStart`, it's unnecessary to check if `err` is nil before consuming gas with `contract.UseGas()`, as `err` will always be nil at that point.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
x/evm/keeper/call_contract.go
Outdated
| blockTxIdx := uint64(txConfig.TxIndex) + 1 | ||
| k.EvmState.BlockTxIndex.Set(ctx, blockTxIdx) | ||
| // TODO: remove after migrating logs | ||
| //err = k.EmitLogEvents(ctx, evmResp) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
x/evm/keeper/call_contract.go (2)
72-74: Improve defer pattern documentationThe comment doesn't accurately describe the purpose of the defer pattern. It's specifically handling panic recovery for out-of-gas scenarios, not just adding "extra information".
- // This is a `defer` pattern to add behavior that runs in the case that the - // error is non-nil, creating a concise way to add extra information. + // Defer pattern to recover from out-of-gas panics and convert them to errors. + // This ensures graceful handling of gas exhaustion scenarios.
117-129: Consider consolidating gas management logicThe gas management logic is spread across multiple error paths. Consider extracting this into a helper method to improve maintainability and reduce duplication.
+func (k Keeper) handleFailedExecution(ctx sdk.Context, evmResp *evm.MsgEthereumTxResponse, gasLimit uint64) error { + k.ResetGasMeterAndConsumeGas(ctx, evmResp.GasUsed) + if strings.Contains(evmResp.VmError, vm.ErrOutOfGas.Error()) { + return fmt.Errorf("gas required exceeds allowance (%d)", gasLimit) + } + if evmResp.VmError == vm.ErrExecutionReverted.Error() { + return fmt.Errorf("VMError: %w", evm.NewExecErrorWithReason(evmResp.Ret)) + } + return fmt.Errorf("VMError: %s", evmResp.VmError) +}x/evm/precompile/test/export.go (1)
211-215: Optimize JSON parsing to avoid double unmarshaling.The code currently unmarshals the same data twice - first to validate the basic structure and then to get the typed response. Consider combining these operations to improve efficiency.
- var wasmMsg wasm.RawContractMessage - s.NoError(json.Unmarshal(queryResp, &wasmMsg)) - s.NoError(wasmMsg.ValidateBasic()) - - var typedResp QueryMsgCountResp - s.NoError(json.Unmarshal(queryResp, &typedResp)) + var typedResp QueryMsgCountResp + s.NoError(json.Unmarshal(queryResp, &typedResp)) + + // If basic validation is required, perform it on the raw bytes + var wasmMsg wasm.RawContractMessage + wasmMsg = wasm.RawContractMessage(queryResp) + s.NoError(wasmMsg.ValidateBasic())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
x/evm/keeper/call_contract.go(1 hunks)x/evm/precompile/test/export.go(5 hunks)
🔇 Additional comments (7)
x/evm/keeper/call_contract.go (3)
32-48: Implementation looks good!
The method effectively handles contract calls by:
- Packing ABI arguments
- Delegating execution to CallContractWithInput
- Using proper error wrapping with %w for error chain preservation
140-144: Address TODO and commented code
There's a TODO comment about log migration and commented-out code for log event emission. This should either be implemented or removed to maintain code clarity.
Let's check if this is tracked:
#!/bin/bash
# Check if there's an issue tracking the log migration
gh issue list --search "in:title log migration"
# Check for other instances of this commented code
rg -A 2 "EmitLogEvents"Would you like me to help create an issue to track this log migration task?
146-148: Remove or uncomment block transaction index update
These commented lines appear to be important for maintaining block transaction state. They should either be uncommented or removed with a clear explanation.
Let's check if this functionality is handled elsewhere:
x/evm/precompile/test/export.go (4)
24-29: LGTM! Well-structured gas limit constants.
The gas limits follow a logical hierarchy and the comment clearly indicates their test-only usage. The query gas limit is appropriately set lower than instantiate/execute operations.
74-79: LGTM! Appropriate gas limit for contract instantiation.
The function correctly uses WasmGasLimitInstantiate for contract instantiation calls and properly handles the response.
185-190: LGTM! Appropriate gas limit for query operations.
The function correctly uses WasmGasLimitQuery for view operations and properly handles the response.
311-319: LGTM! Proper gas management for execute operations.
The function correctly:
- Resets the gas meter before contract execution
- Uses
WasmGasLimitExecutefor execute operations - Properly handles the response
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
x/evm/errors.go (1)
54-56: Enhance function documentation for better clarity.The current documentation could be more descriptive. Consider adding details about:
- What conditions trigger a revert in the EVM context
- The expected format of the
revertReasonparameter- How this relates to EVM execution and error handling
-// NewRevertError unpacks the revert return bytes and returns a wrapped error -// with the return reason. +// NewRevertError creates an error from EVM revert data. When a contract execution +// reverts, it can provide a reason (typically an ABI-encoded error message). +// This function attempts to unpack that reason into a human-readable format. +// +// Parameters: +// - revertReason: ABI-encoded revert data from the EVM +// +// Returns: +// - A formatted error containing the unpacked revert reason, or an error +// indicating that unpacking failedx/evm/keeper/erc20.go (2)
79-82: Improve error handling consistency.When returning early due to error, consider using a more consistent pattern:
-return balanceIncrease, nil, errors.Wrap(err, "failed to retrieve recipient balance") +return nil, nil, errors.Wrap(err, "failed to retrieve recipient balance")
171-178: Consider reducing duplication in ERC20 loading methods.The error handling and contract calling patterns are duplicated across
LoadERC20String,loadERC20Uint8, andLoadERC20BigInt. Consider extracting the common pattern into a helper function.func (k Keeper) callAndUnpack( ctx sdk.Context, abi *gethabi.ABI, contract gethcommon.Address, methodName string, result interface{}, args ...any, ) error { res, err := k.CallContract( ctx, abi, evm.EVM_MODULE_ADDRESS, &contract, false, Erc20GasLimitQuery, methodName, args..., ) if err != nil { return err } return abi.UnpackIntoInterface(result, methodName, res.Ret) }Also applies to: 203-206, 234-236
x/evm/keeper/msg_server.go (1)
364-375: Improve readability of gas refund calculations.The gas refund logic is complex and could benefit from better variable naming and inline documentation.
Consider this refactoring to improve clarity:
-refundQuotient := params.RefundQuotientEIP3529 // EIP-3529: refunds are capped to gasUsed / 5 -minGasUsedPct := k.GetMinGasUsedMultiplier(ctx) // Evmos invention: https://github.com/evmos/ethermint/issues/1085 -if fullRefundLeftoverGas { - refundQuotient = 1 // 100% refund - minGasUsedPct = math.LegacyZeroDec() // no minimum, get the actual gasUsed value -} -temporaryGasUsed := msg.Gas() - leftoverGas -refund := GasToRefund(stateDB.GetRefund(), temporaryGasUsed, refundQuotient) - -// update leftoverGas and temporaryGasUsed with refund amount -leftoverGas += refund -temporaryGasUsed -= refund +// Calculate refund parameters based on call type +// For internal calls (fullRefundLeftoverGas=true), allow 100% refund with no minimum gas requirement +// For user calls, cap refund at 20% of used gas (EIP-3529) +refundCapDivisor := params.RefundQuotientEIP3529 // Default: refund capped at gasUsed/5 (20%) +minRequiredGasPercent := k.GetMinGasUsedMultiplier(ctx) +if fullRefundLeftoverGas { + refundCapDivisor = 1 // Allow 100% refund + minRequiredGasPercent = math.LegacyZeroDec() // No minimum requirement +} + +// Calculate initial gas consumption and potential refund +initialGasUsed := msg.Gas() - leftoverGas +refundAmount := GasToRefund(stateDB.GetRefund(), initialGasUsed, refundCapDivisor) + +// Apply refund +leftoverGas += refundAmount +finalGasUsed := initialGasUsed - refundAmountx/evm/keeper/grpc_query.go (1)
448-457: Fix typo in error message and improve error handling structure.There's a typo in the error message and the error handling structure could be improved.
Apply these changes:
- return nil, fmt.Errorf("Estimate gas VMError: %w", evm.NewRevertError(result.Ret)) + return nil, fmt.Errorf("estimate gas VMError: %w", evm.NewRevertError(result.Ret)) - return nil, fmt.Errorf("Estimgate gas VMError: %s", result.VmError) + return nil, fmt.Errorf("estimate gas VMError: %s", result.VmError)Consider consolidating the error handling into a helper function to improve readability:
func handleVMError(result *evm.MsgEthereumTxResponse, gasCap uint64) error { if result.VmError == vm.ErrExecutionReverted.Error() { return fmt.Errorf("estimate gas VMError: %w", evm.NewRevertError(result.Ret)) } if result.VmError == vm.ErrOutOfGas.Error() { return fmt.Errorf("gas required exceeds allowance (%d)", gasCap) } return fmt.Errorf("estimate gas VMError: %s", result.VmError) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
eth/rpc/backend/call_tx.go(1 hunks)x/evm/errors.go(1 hunks)x/evm/keeper/call_contract.go(1 hunks)x/evm/keeper/erc20.go(8 hunks)x/evm/keeper/erc20_test.go(3 hunks)x/evm/keeper/gas_fees.go(1 hunks)x/evm/keeper/grpc_query.go(6 hunks)x/evm/keeper/keeper.go(3 hunks)x/evm/keeper/msg_server.go(11 hunks)x/evm/msg.go(4 hunks)x/evm/precompile/funtoken_test.go(8 hunks)x/evm/precompile/oracle_test.go(2 hunks)x/evm/precompile/test/export.go(5 hunks)x/evm/precompile/wasm_test.go(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/evm/keeper/gas_fees.go
🚧 Files skipped from review as they are similar to previous changes (6)
- x/evm/keeper/call_contract.go
- x/evm/keeper/erc20_test.go
- x/evm/keeper/keeper.go
- x/evm/precompile/funtoken_test.go
- x/evm/precompile/oracle_test.go
- x/evm/precompile/test/export.go
🔇 Additional comments (21)
x/evm/errors.go (1)
57-63: LGTM! Clean and effective error handling implementation.
The implementation is well-structured with:
- Clear error messages for both successful and failed unpacking
- Proper handling of unpacking errors
- Simplified error creation using standard
fmt.Errorf
x/evm/keeper/erc20.go (2)
Line range hint 85-120: Great improvement in balance verification!
The new balance verification logic is a significant improvement:
- Handles fee-on-transfer tokens correctly
- Verifies actual balance changes
- Provides detailed error messages
17-27: Verify gas limit constants against production usage patterns.
The gas limits seem reasonable, but it would be good to verify these against real transaction patterns:
Erc20GasLimitDeploy: 2M gas for deploymentErc20GasLimitQuery: 100K gas for queriesErc20GasLimitExecute: 200K gas for transfers/mint/burn
✅ Verification successful
Let me search for actual ERC20 contract interactions and gas usage patterns to verify these constants.
Based on the search results, I can now generate a final response as we have sufficient evidence from the codebase usage:
Gas limits are properly calibrated based on actual usage patterns
The gas limits are verified to be appropriate based on their usage across the codebase:
Erc20GasLimitExecute(200K) is consistently used for mint/transfer/burn operations across multiple test files and implementationsErc20GasLimitQuery(100K) is used for read-only operations like querying token detailsErc20GasLimitDeploy(2M) is used during contract deployment infuntoken_from_coin.go
The presence of malicious contract test cases (TestERC20MaliciousTransfer.sol, TestERC20MaliciousName.sol) suggests these limits have been tested against edge cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing gas usage patterns in tests
rg -A 2 "gas.*used" test/
Length of output: 73
Script:
#!/bin/bash
# Let's search for gas-related tests and configurations in the entire codebase
rg -i "gas.*limit.*erc20|erc20.*gas.*limit" -A 2
# Also search for any ERC20-related test files
fd -e go -e sol test
Length of output: 9487
Script:
#!/bin/bash
# Search for ERC20 contract calls that might show gas usage
ast-grep --pattern 'func ($_ *Keeper) $_($_ context.Context, $$$) {
$$$
gas := $$$
$$$
}'
# Look for any gas-related comments or documentation
rg -i "gas.*limit" README.md docs/
Length of output: 180
eth/rpc/backend/call_tx.go (1)
298-301: LGTM! Improved error handling for VM execution results.
The enhanced error handling properly differentiates between execution reverts and other VM errors:
- Execution reverts now correctly return the revert data, allowing contracts to decode the reason
- Other VM errors are properly wrapped as internal errors
This change improves compatibility with Ethereum's error handling patterns and enables better debugging of failed transactions.
x/evm/precompile/wasm_test.go (6)
22-26: LGTM: Well-defined gas limit constants.
The introduction of WasmGasLimitQuery and WasmGasLimitExecute constants improves code maintainability by centralizing gas limit values. The values are reasonable for their respective operations.
51-63: LGTM: Consistent gas limit usage in contract calls.
Both contract calls appropriately use WasmGasLimitExecute for execute operations, maintaining consistency in gas management.
Also applies to: 82-94
138-138: LGTM: Proper gas meter management in query operation.
The gas meter is correctly reset before the contract call, ensuring accurate gas measurement. The use of WasmGasLimitQuery is appropriate for this query operation.
Also applies to: 140-151
313-321: LGTM: Consistent gas limit in error test cases.
The test case maintains consistency by using WasmGasLimitExecute while validating error scenarios.
446-454: LGTM: Proper gas management in multi-execution validation.
The test maintains consistent gas management by using WasmGasLimitExecute for multi-message execution scenarios.
492-500: LGTM: Consistent gas management in partial execution test.
The test properly uses WasmGasLimitExecute while verifying partial execution behavior.
x/evm/msg.go (2)
18-18: LGTM: Standardized error handling import
The change from errortypes to sdkerrors aligns with Cosmos SDK's standard error handling patterns.
157-157: LGTM: Improved error handling in ValidateBasic
The error handling improvements in ValidateBasic enhance transaction validation by:
- Using standardized error types from
sdkerrors - Providing clear error messages
- Adding essential checks for gas-related parameters
Also applies to: 169-169, 174-174, 184-184
x/evm/keeper/msg_server.go (4)
65-103: LGTM: Improved error handling and code organization.
The changes enhance error handling for gas management and improve code organization by splitting event emission into separate methods.
408-408: LGTM: Improved error message clarity.
The error message now clearly explains the minimum transfer requirement and unit conversion.
539-539: LGTM: Enhanced gas management and error handling.
The changes appropriately handle gas limits for ERC20 operations and improve error handling in token transfers.
Also applies to: 592-592
687-704: 🛠️ Refactor suggestion
Improve error handling consistency in EmitLogEvents.
While JSON marshaling errors are properly handled, the final event emission error is silently ignored.
Apply this change for consistent error handling:
-_ = ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{TxLogs: txLogs})
+if err := ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{TxLogs: txLogs}); err != nil {
+ return errors.Wrap(err, "failed to emit tx log event")
+}Likely invalid or redundant comment.
x/evm/keeper/grpc_query.go (5)
287-287: LGTM: ApplyEvmMsg parameter addition is correct.
The addition of false as the final parameter to ApplyEvmMsg is consistent with the PR's gas management enhancements and maintains the existing behavior for eth_call operations.
326-329: LGTM: Improved error handling for EVM config.
The addition of specific error handling for EVM config loading improves error reporting.
341-344: LGTM: Proper nonce handling implementation.
The addition of nonce handling with clear documentation improves the code's correctness and maintainability.
523-523: LGTM: Consistent ApplyEvmMsg parameter addition.
The addition of false as the final parameter to ApplyEvmMsg maintains consistency with other similar calls in the codebase.
805-805: LGTM: Consistent ApplyEvmMsg parameter addition.
The addition of false as the final parameter to ApplyEvmMsg maintains consistency with other similar calls in the codebase.
|
|
||
| // RevertError is an API error that encompass an EVM revert with JSON error | ||
| // code and a binary data blob. |
There was a problem hiding this comment.
💡 Codebase verification
RevertError struct should be removed as it's unused
The RevertError struct is defined but never actually used in the codebase. The NewRevertError function is actively used in three locations to handle EVM execution reverts, but it returns a regular error type, not the RevertError struct. The struct appears to be a leftover artifact that can be safely removed.
- Remove the unused
RevertErrorstruct definition inx/evm/errors.go
🔗 Analysis chain
Verify if RevertError struct can be removed.
The RevertError struct appears to be unused after the refactoring of NewRevertError. Let's verify if it can be safely removed.
Let me analyze the usage of RevertError more thoroughly by checking its implementation and the contexts where it's used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usages of RevertError in the codebase
echo "Searching for RevertError usage..."
rg "RevertError" --type go
# Search for any tests that might depend on this type
echo "Searching for test files using RevertError..."
rg "RevertError" --type go -g "*_test.go"
Length of output: 823
Script:
#!/bin/bash
# Check the implementation of RevertError struct and NewRevertError
echo "RevertError implementation:"
cat x/evm/errors.go
# Check the context of usage in call_contract.go
echo -e "\nUsage in call_contract.go:"
rg -B 2 -A 2 "NewRevertError" x/evm/keeper/call_contract.go
# Check the context of usage in grpc_query.go
echo -e "\nUsage in grpc_query.go:"
rg -B 2 -A 2 "NewRevertError" x/evm/keeper/grpc_query.go
# Check the context of usage in backend/call_tx.go
echo -e "\nUsage in backend/call_tx.go:"
rg -B 2 -A 2 "NewRevertError" eth/rpc/backend/call_tx.go
Length of output: 3609
| @@ -635,20 +646,9 @@ func (k *Keeper) EmitEthereumTxEvents( | |||
| } | |||
| err := ctx.EventManager().EmitTypedEvent(eventEthereumTx) | |||
| if err != nil { | |||
| return errors.Wrap(err, "failed to emit event ethereum tx") | |||
| return errors.Wrap(err, "EmitEthereumTxEvents: failed to emit event ethereum tx") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent error handling in event emissions.
While error handling is added for the typed event, subsequent event emissions using EmitTypedEvent don't check for errors. Consider handling errors consistently across all event emissions.
Apply this pattern to all event emissions:
-_ = ctx.EventManager().EmitTypedEvent(&evm.EventContractDeployed{
+if err := ctx.EventManager().EmitTypedEvent(&evm.EventContractDeployed{
Sender: msg.From().Hex(),
ContractAddr: contractAddr.String(),
-})
+}); err != nil {
+ return errors.Wrap(err, "failed to emit contract deployed event")
+}Also applies to: 665-683
| } | ||
| return &RevertError{ | ||
| error: err, | ||
| reason: hexutil.Encode(result), |
There was a problem hiding this comment.
In this code block, you can get a nil unpack error and still have an empty reason. It should still handle that case
| @@ -193,7 +193,7 @@ func (s *NodeSuite) Test_EstimateGas() { | |||
| } { | |||
| msg.Value = msgValue | |||
| _, err = s.ethClient.EstimateGas(context.Background(), msg) | |||
| s.ErrorContains(err, "StateDB: wei amount is too small") | |||
| s.ErrorContains(err, "wei amount is too small") | |||
There was a problem hiding this comment.
💡 Codebase verification
Error message inconsistency found in wei amount validation
The error message format is inconsistent across the codebase:
x/evm/keeper/msg_server.goandx/evm/const.gouse a detailed format with explanation: "wei amount is too small (%s), cannot transfer less than 1 micronibi..."x/evm/keeper/statedb_test.goandeth/rpc/rpcapi/eth_api_test.gouse a simpler format: "wei amount is too small"
The test files should verify against the actual error messages used in the implementation.
🔗 Analysis chain
LGTM! Verify error message consistency.
The simplified error message improves clarity. However, let's ensure this error message is consistently used across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistent error messages related to "wei amount is too small"
# across the codebase to ensure consistent error reporting.
# Search for variations of the error message
echo "Searching for variations of 'wei amount is too small' error message:"
rg -i "wei amount.*too small|small.*wei amount"
# Search for "StateDB:" prefix in error messages to ensure it's removed everywhere
echo -e "\nChecking for any remaining 'StateDB:' prefixes in error messages:"
rg "StateDB:"
Length of output: 1027
Notes
Example:
gasLimittoCallContractWithInputto limit gas for internal calls related to funtoken.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Performance Improvements
Documentation
Tests