fix: make gas refunds error free and panic safe #96
Conversation
WalkthroughThe changes in this pull request involve multiple modifications across several files, primarily focusing on the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
x/evm/keeper/txutils.go (2)
228-230: LGTM! Consider improving readability with a named variable.The fee validation logic is now more precise and safer, correctly handling both empty fees and single-denomination cases. This change helps prevent potential issues with multiple fees or mixed denominations.
Consider improving readability by extracting the condition to a named variable:
- if !(len(fees) == 0 || (len(fees) == 1 && fees[0].Denom == params.FeeDenom)) { + isValidFee := len(fees) == 0 || (len(fees) == 1 && fees[0].Denom == params.FeeDenom) + if !isValidFee { return nil, nil, nil }
406-408: LGTM! Consistent with previous fee validation.The fee validation logic matches the implementation in
ConvertCosmosTxToEthereumTx, maintaining consistency across the codebase.Consider applying the same readability improvement here:
- if !(len(fees) == 0 || (len(fees) == 1 && fees[0].Denom == params.FeeDenom)) { + isValidFee := len(fees) == 0 || (len(fees) == 1 && fees[0].Denom == params.FeeDenom) + if !isValidFee { return false, nil }app/posthandler/gasrefund.go (1)
96-97: Verify the appropriateness ofgasLimitForRefundvalueThe constant
gasLimitForRefundis set to1_000_000. Consider verifying if this gas limit is sufficient for all refund scenarios without being excessively high, which could impact overall gas consumption.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app/app.go(1 hunks)app/posthandler/gasrefund.go(3 hunks)app/posthandler/posthandler.go(1 hunks)x/evm/keeper/txutils.go(2 hunks)
🔇 Additional comments (6)
app/posthandler/posthandler.go (4)
6-6: LGTM: Appropriate logging package import.
The addition of cosmossdk.io/log is aligned with the PR's objective to make gas refunds more robust by adding logging capabilities.
Line range hint 22-26: LGTM: EVMKeeper interface remains stable.
The interface maintains the essential methods needed for gas refund operations while keeping the contract focused and cohesive.
15-16: Verify the removal of AccountKeeper dependency.
The signature change from AccountKeeper to log.Logger appears to be a positive architectural change, reducing coupling. However, we should verify that the AccountKeeper wasn't being used for any critical gas refund validations.
✅ Verification successful
The AccountKeeper removal is safe and improves the code
Based on the git history and code examination, the removal of AccountKeeper is verified to be safe because:
- AccountKeeper was never used in the gas refund logic
- The change actually improves error handling by:
- Adding proper logging through the new logger parameter
- Introducing a safer refund mechanism with
safeRefundmethod - Adding panic recovery and proper gas limits for refund operations
The gas refund functionality continues to work through EVMKeeper and ERC20Keeper, which handle all the necessary token operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if AccountKeeper was used for any gas refund related operations
# in the previous version of the code
# Search for any gas refund related code that might have used AccountKeeper
rg -B 5 -A 5 "AccountKeeper.*[Gg]as[Rr]efund"
# Search for any potential validation or checks in the old code
rg -B 5 -A 5 "GasRefundDecorator.*AccountKeeper"
Length of output: 98
Script:
#!/bin/bash
# Let's try a broader search to understand the context and usage of AccountKeeper
# Search for any references to AccountKeeper in the file
rg -B 5 -A 5 "AccountKeeper" app/posthandler/posthandler.go
# Search for any gas refund related code in the file
rg -B 5 -A 5 "[Gg]as[Rr]efund" app/posthandler/posthandler.go
# Look at the git history of the file to see the changes
git log -p app/posthandler/posthandler.go
# Check for any tests that might reveal the usage
fd posthandler_test.go --exec rg -B 5 -A 5 "AccountKeeper" {}
Length of output: 2456
Script:
#!/bin/bash
# Let's examine the GasRefundDecorator implementation to see how it was modified
# when AccountKeeper was removed
# Look at the current implementation
rg -B 10 -A 10 "type GasRefundDecorator" app/posthandler/
# Check the git history of the gas refund decorator
git log -p app/posthandler/gas_refund.go
Length of output: 1427
Script:
#!/bin/bash
# The file name seems to be gasrefund.go, not gas_refund.go. Let's check its history
# Look at the git history of the correct file
git log -p app/posthandler/gasrefund.go
# Also check the implementation of PostHandle method
rg -B 10 -A 20 "func \(.*\) PostHandle" app/posthandler/gasrefund.go
Length of output: 6735
19-19: Ensure proper error logging in GasRefundDecorator.
The updated NewGasRefundDecorator now receives a logger, which aligns with making the gas refunds error-free. However, we should verify that the decorator properly logs errors and handles panics.
✅ Verification successful
Error logging and panic handling are properly implemented in GasRefundDecorator
The GasRefundDecorator has robust error handling and logging:
- Errors are properly logged in
safeRefundmethod:- Out of gas errors are caught and logged:
erd.logger.Error("failed to refund gas", "err", r) - Failed refund operations are logged:
erd.logger.Error("failed to refund gas", "err", err)
- Out of gas errors are caught and logged:
- Panic recovery is implemented using defer/recover pattern
- Error conditions are properly handled and propagated in PostHandle method
- The logger is properly initialized with module context:
logger.With("module", "gas_refund")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in GasRefundDecorator
# Check the implementation of GasRefundDecorator for error logging
ast-grep --pattern 'func NewGasRefundDecorator($_, $_) {
$$$
}'
# Look for panic handling and error logging patterns
rg -A 10 "logger\.(Error|Debug|Info).*[Gg]as[Rr]efund"
Length of output: 124
Script:
#!/bin/bash
# Let's try a broader search for the GasRefundDecorator implementation and usage
# Search for the GasRefundDecorator type and implementation
ast-grep --pattern 'type GasRefundDecorator struct {
$$$
}'
# Search for any method implementations on GasRefundDecorator
ast-grep --pattern 'func ($_ *GasRefundDecorator) $_($$$) $$$'
# Look for the constructor with a different pattern
ast-grep --pattern 'func NewGasRefundDecorator'
# Broader search for any gas refund related logging
rg -A 5 "[Gg]as[Rr]efund"
Length of output: 19067
app/app.go (1)
370-370: LGTM! Improved error handling for gas refunds.
The change from AccountKeeper to Logger enhances the robustness of gas refund operations by:
- Adding logging capabilities for better error tracking
- Reducing coupling by removing direct dependency on
AccountKeeper
Let's verify the posthandler implementation:
✅ Verification successful
Verified: Logger change improves gas refund robustness
The change from AccountKeeper to Logger is correctly implemented and enhances the gas refund mechanism by:
- Adding proper error logging in the
safeRefundfunction for better error tracking - Using panic recovery with logging to handle out-of-gas scenarios gracefully
- Removing direct dependency on
AccountKeeperand usingERC20Keeperfor coin operations
The implementation aligns with the PR's goal of making gas refunds error-free and panic-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the posthandler correctly implements the new logger-based interface
# and that AccountKeeper removal doesn't break existing functionality
# Check the posthandler implementation
ast-grep --pattern 'func NewPostHandler($_) {
$$$
}'
# Look for any remaining AccountKeeper references in posthandler
rg -A 5 "AccountKeeper" "app/posthandler"
Length of output: 96
Script:
#!/bin/bash
# Let's try a broader search to locate the posthandler implementation and related files
# Find posthandler related files
fd posthandler
# Search for NewPostHandler function definition in the entire codebase
rg -A 10 "func NewPostHandler"
# Search for any AccountKeeper usage in the entire codebase
rg -A 5 "AccountKeeper"
Length of output: 42748
Script:
#!/bin/bash
# Let's check the posthandler implementation and gas refund decorator to verify the logger usage
# Find and check the gas refund decorator implementation
rg -A 10 "NewGasRefundDecorator"
# Check the entire posthandler.go file content to understand the full context
cat app/posthandler/posthandler.go
Length of output: 1958
Script:
#!/bin/bash
# Let's check the gas refund decorator implementation to verify the logger usage
# Check the gasrefund.go implementation
cat app/posthandler/gasrefund.go
Length of output: 3265
app/posthandler/gasrefund.go (1)
25-28: Ensure all invocations of NewGasRefundDecorator are updated with the new logger parameter
The function NewGasRefundDecorator now requires a logger log.Logger parameter. Please verify that all places where this function is called have been updated to pass the appropriate logger instance to prevent compilation errors.
Run the following script to identify any calls that may not have been updated:
✅ Verification successful
All usages of NewGasRefundDecorator have been properly updated
The search results show that there is only one invocation of NewGasRefundDecorator in app/posthandler/posthandler.go, and it's already being called with the logger parameter:
NewGasRefundDecorator(logger, ek),This matches the new function signature defined in app/posthandler/gasrefund.go. No other usages were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all invocations of NewGasRefundDecorator without the logger parameter.
# Search for calls to NewGasRefundDecorator with a missing logger argument.
rg -A 2 'NewGasRefundDecorator\([^,]*\)'
Length of output: 42
Script:
#!/bin/bash
# Let's try a broader search to find all usages of NewGasRefundDecorator
rg -l "NewGasRefundDecorator"
Length of output: 89
Script:
#!/bin/bash
# Let's also check the implementation and usage in test files
rg -A 5 "NewGasRefundDecorator"
Length of output: 695
Script:
#!/bin/bash
# Search for any test files related to gasrefund
fd "gasrefund.*test.*"
Length of output: 24
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
==========================================
+ Coverage 28.07% 28.29% +0.21%
==========================================
Files 126 126
Lines 14021 14029 +8
==========================================
+ Hits 3937 3969 +32
+ Misses 9531 9499 -32
- Partials 553 561 +8
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
app/posthandler/gasrefund.go (3)
25-29: Consider adding logger validationThe constructor changes look good, but consider adding a nil check for the logger parameter to prevent potential runtime panics.
func NewGasRefundDecorator(logger log.Logger, ek EVMKeeper) sdk.PostDecorator { + if logger == nil { + panic("logger cannot be nil") + } logger = logger.With("module", "gas_refund") return &GasRefundDecorator{ logger, ek, } }
96-96: Consider making gas limit configurableThe hardcoded gas limit of 1,000,000 for refund operations should ideally be configurable to allow for adjustments without code changes.
+const defaultGasLimitForRefund = 1_000_000 + type GasRefundDecorator struct { logger log.Logger ek EVMKeeper + gasLimitForRefund uint64 } func NewGasRefundDecorator(logger log.Logger, ek EVMKeeper) sdk.PostDecorator { logger = logger.With("module", "gas_refund") return &GasRefundDecorator{ logger, ek, + defaultGasLimitForRefund, } }
88-88: Fix typo in error log messageThere's a typo in the error log message parameter name.
-erd.logger.Error("failed to refund gas", "err", r, "feePayer", feePayer, "refudnAmount", coinsRefund) +erd.logger.Error("failed to refund gas", "err", r, "feePayer", feePayer, "refundAmount", coinsRefund)app/posthandler/posthandler_test.go (3)
33-41: Add field documentation to improve test suite clarity.While the struct is documented, adding documentation for individual fields would improve maintainability and clarity.
type PostHandlerTestSuite struct { suite.Suite - app *minievmapp.MinitiaApp - ctx sdk.Context - clientCtx client.Context - txBuilder client.TxBuilder + // app is the Minitia application instance used for testing + app *minievmapp.MinitiaApp + // ctx is the context for the test application + ctx sdk.Context + // clientCtx is the client context used for transaction handling + clientCtx client.Context + // txBuilder is used to construct test transactions + txBuilder client.TxBuilder }
64-79: Add validation checks in SetupTest.Consider adding validation checks to ensure the test environment is properly initialized.
func (suite *PostHandlerTestSuite) SetupTest() { tempDir := suite.T().TempDir() suite.app, suite.ctx = suite.createTestApp(tempDir) + + // Validate essential components are initialized + suite.Require().NotNil(suite.app, "app should not be nil") + suite.Require().NotNil(suite.ctx, "context should not be nil") suite.ctx = suite.ctx.WithBlockHeight(1) // Set up TxConfig. encodingConfig := minievmapp.MakeEncodingConfig() + suite.Require().NotNil(encodingConfig.TxConfig, "tx config should not be nil")
1-136: Add test cases for gas refund functionality.While the test infrastructure is well-implemented, there are no specific test cases for gas refunds. Given that the PR's objective is to make gas refunds error-free and panic-safe, consider adding test cases that verify:
- Gas refund calculation correctness
- Error handling in gas refund scenarios
- Panic safety mechanisms
Would you like me to help generate test cases for these scenarios?
app/posthandler/gasrefund_test.go (4)
26-30: Add test documentation to improve clarity.Consider adding a doc comment to explain:
- The purpose of this test
- The expected behavior
- The test's prerequisites and setup requirements
+// Test_NotSpendingGasForTxWithFeeDenom verifies that: +// 1. Gas refunds are correctly calculated when not all gas is consumed +// 2. Refunds are properly credited to the sender's account +// 3. The refund amount matches the gas refund ratio defined in params func (suite *PostHandlerTestSuite) Test_NotSpendingGasForTxWithFeeDenom() {
36-47: Define decimals as a constant.The decimal value of 18 appears to be a standard value. Consider defining it as a named constant for better maintainability and clarity.
+const ( + // StandardERC20Decimals represents the standard number of decimals for ERC20 tokens + StandardERC20Decimals = uint8(18) +) + // create fee token -decimals := uint8(18) +decimals := StandardERC20Decimals
87-96: Improve error handling for key generation and signing.Consider wrapping the key generation and signing operations in a helper function with proper error handling and cleanup.
+func generateAndSignTx(tx *coretypes.Transaction, chainID *big.Int) (*coretypes.Transaction, *ecdsa.PrivateKey, error) { + privKey, err := ecdsa.GenerateKey(crypto.S256(), rand.Reader) + if err != nil { + return nil, nil, fmt.Errorf("failed to generate private key: %w", err) + } + + signer := coretypes.LatestSignerForChainID(chainID) + signedTx, err := coretypes.SignTx(tx, signer, privKey) + if err != nil { + return nil, nil, fmt.Errorf("failed to sign transaction: %w", err) + } + + return signedTx, privKey, nil +}
122-126: Make the refund verification more explicit.The current verification could be more clear about the expected values and the calculation process.
-// Check the gas refund +// Calculate and verify the gas refund +// Expected refund = (gasPrice * gasRefundRatio * consumedGas) amount := suite.app.BankKeeper.GetBalance(ctx, sender, params.FeeDenom) -refunds, _ := gasPrices.MulDec(gasRefundRatio.MulInt64(int64(gasLimit / 2))).TruncateDecimal() -suite.Equal(amount, refunds[0]) +consumedGas := int64(gasLimit / 2) +expectedRefunds, _ := gasPrices.MulDec(gasRefundRatio.MulInt64(consumedGas)).TruncateDecimal() +suite.Equal(expectedRefunds[0], amount, "Gas refund amount should match expected calculation")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/posthandler/gasrefund.go(3 hunks)app/posthandler/gasrefund_test.go(1 hunks)app/posthandler/posthandler_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/posthandler/gasrefund.go
[warning] 86-90: app/posthandler/gasrefund.go#L86-L90
Added lines #L86 - L90 were not covered by tests
[warning] 103-105: app/posthandler/gasrefund.go#L103-L105
Added lines #L103 - L105 were not covered by tests
🔇 Additional comments (5)
app/posthandler/gasrefund.go (3)
7-7: LGTM: Logger integration looks good!
The addition of the logger field and its import enhances observability for gas refund operations, which aligns well with making the system more robust and debuggable.
Also applies to: 21-22
76-77: LGTM: Good separation of concerns
Moving the refund logic to a separate method improves code organization and error handling.
102-105: Consider propagating errors instead of silent logging
The current implementation silently logs errors from SendCoins without propagating them. Consider whether this aligns with the error handling strategy of the codebase.
#!/bin/bash
# Check error handling patterns in other decorators
rg "SendCoins.*return err" --type goIf other decorators propagate errors, consider maintaining consistency:
err := erd.ek.ERC20Keeper().SendCoins(cacheCtx, feeCollectorAddr, feePayer, coinsRefund)
if err != nil {
- erd.logger.Error("failed to refund gas", "err", err)
- return
+ erd.logger.Error("failed to refund gas", "err", err, "feePayer", feePayer, "refundAmount", coinsRefund)
+ return fmt.Errorf("failed to refund gas: %w", err)
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 103-105: app/posthandler/gasrefund.go#L103-L105
Added lines #L103 - L105 were not covered by tests
app/posthandler/posthandler_test.go (2)
1-31: LGTM: Package structure and imports are well organized.
The package structure follows Go conventions, and all imports are properly organized and necessary for the test suite.
134-136: LGTM: Test runner is properly implemented.
The test runner correctly uses the testify suite framework.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation