feat: introduce hook max gas #119
Conversation
WalkthroughThis pull request introduces several changes primarily focused on enhancing gas management for the execution of hooks within the token deposit process. A new field, Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #119 +/- ##
==========================================
+ Coverage 49.25% 49.37% +0.12%
==========================================
Files 57 57
Lines 4233 4249 +16
==========================================
+ Hits 2085 2098 +13
- Misses 1719 1721 +2
- Partials 429 430 +1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
x/opchild/types/params.go (2)
13-13: Add documentation for the DefaultHookMaxGas constant.Please add a comment explaining why 1,000,000 was chosen as the default value for hook max gas. This helps future maintainers understand the reasoning behind this specific limit.
Line range hint
51-70: Add validation for HookMaxGas in the Validate method.The Validate method should check if HookMaxGas is within acceptable bounds to prevent potential issues with hook execution.
Consider adding validation like this:
func (p Params) Validate(ac address.Codec) error { if _, err := ac.StringToBytes(p.Admin); err != nil { return err } for _, be := range p.BridgeExecutors { if _, err := ac.StringToBytes(be); err != nil { return err } } if err := p.MinGasPrices.Validate(); err != nil { return err } if p.MaxValidators == 0 { return ErrZeroMaxValidators } + if p.HookMaxGas == 0 { + return fmt.Errorf("hook max gas cannot be zero") + } + // Validate fee whitelist addresses for _, addr := range p.FeeWhitelist { if _, err := ac.StringToBytes(addr); err != nil { return err } } return nil }proto/opinit/opchild/v1/types.proto (1)
50-51: Enhance the field documentation.The current documentation could be more descriptive. Consider adding:
- Default value
- Acceptable range/constraints
- Impact on hook execution
- Relationship with system-wide gas limits
Example:
- // Max gas for hook execution of `MsgFinalizeTokenDeposit` + // Maximum gas limit for hook execution during MsgFinalizeTokenDeposit processing. + // Must be positive and not exceed the block gas limit. + // Default: 1000000 gas units. + // Note: Hook execution will fail if gas consumption exceeds this limit.x/opchild/keeper/msg_server_test.go (3)
563-618: LGTM! Comprehensive test coverage for out-of-gas scenario.The test thoroughly validates the behavior when a hook fails due to insufficient gas by:
- Setting an extremely low gas limit (1)
- Verifying the failure event attributes
- Checking that no token transfers occurred
- Ensuring proper error handling
Consider adding a comment explaining why the gas limit is set to 1, as it might not be immediately obvious that this is intentionally set to an extremely low value to force the out-of-gas condition.
params.HookMaxGas = 1 +// Set an extremely low gas limit to force an out-of-gas condition input.OPChildKeeper.SetParams(ctx, params)
595-597: Consider adding gas meter validation.The test sets up a gas meter with 2,000,000 gas units but doesn't verify the gas consumption. Consider adding assertions to validate that the hook execution attempted to consume more gas than the 1-unit limit.
601-609: Enhance error message validation.While the test verifies the presence of "panic:" in the error message, it would be more robust to validate the specific out-of-gas error message to ensure the failure is happening for the expected reason.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/opchild/types/types.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
proto/opinit/opchild/v1/types.proto(1 hunks)x/opchild/keeper/deposit.go(2 hunks)x/opchild/keeper/msg_server.go(1 hunks)x/opchild/keeper/msg_server_test.go(2 hunks)x/opchild/types/params.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/opchild/keeper/msg_server.go
[warning] 441-442: x/opchild/keeper/msg_server.go#L441-L442
Added lines #L441 - L442 were not covered by tests
🔇 Additional comments (8)
x/opchild/types/params.go (2)
25-25: LGTM!
The DefaultParams function correctly incorporates the new DefaultHookMaxGas parameter.
30-30: LGTM!
The NewParams function signature and struct initialization are properly updated to include the new HookMaxGas parameter.
Also applies to: 38-38
x/opchild/keeper/deposit.go (3)
7-9: LGTM: Required import added for gas meter functionality
The addition of storetypes import is necessary for the new gas management features.
13-19: LGTM: Gas limit management implementation
The implementation correctly:
- Stores the original gas meter
- Calculates available gas for hook execution
- Caps the gas at hookMaxGas
30-30: Verify gas consumption behavior
The implementation replaces the gas meter and consumes gas in the deferred function. Let's verify this behavior:
- Gas consumption is properly tracked
- Original gas meter is correctly updated
- No gas leaks occur during hook execution
Also applies to: 33-34
✅ Verification successful
Let me gather more specific information about the gas consumption implementation and tests.
Based on the code analysis, I can now generate the final response:
Gas consumption implementation appears correct and well-structured
The implementation shows proper gas handling with safety measures in place:
- Original gas meter is preserved and updated correctly in the deferred function
- Gas limit is properly capped with
hookMaxGas - New gas meter is created with controlled limit for hook execution
- Gas consumption is accurately tracked and transferred back to the original meter
- Panic recovery ensures gas is always properly accounted
While test coverage could be more comprehensive for gas-specific scenarios, the core implementation follows best practices for gas management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests validating gas consumption behavior
rg -l "GasMeter|ConsumeGas" --type go
# Look for potential gas-related test cases
ast-grep --pattern 'func Test$_.*Gas.*\(.*testing\.T\).*{
$$$
}'
Length of output: 172
Script:
#!/bin/bash
# Check the gas consumption implementation in deposit.go
rg -A 10 "GasMeter|ConsumeGas" x/opchild/keeper/deposit.go
# Check test implementations for gas-related scenarios
rg -A 10 "GasMeter|ConsumeGas" x/opchild/keeper/msg_server_test.go
# Look for any gas-related test setup or assertions
ast-grep --pattern 'func \($_\) Test.*\(t \*testing.T\) {
$$$
GasMeter
$$$
}'
Length of output: 1680
proto/opinit/opchild/v1/types.proto (1)
51-51: LGTM! Field implementation looks correct.
The field implementation follows proper protobuf conventions:
- Appropriate field number sequence
- Suitable type (uint64) for gas limits
- Consistent YAML tag format
x/opchild/keeper/msg_server.go (2)
Line range hint 447-456: LGTM! Verify hook execution with different gas limits.
The changes effectively implement gas management for hook execution and provide detailed event attributes for observability. The error handling and recovery logic is comprehensive.
#!/bin/bash
# Description: Verify hook execution implementation across the codebase
# Test: Search for all hook execution calls to ensure consistent gas limit usage
# Check hook execution implementation
echo "Checking hook execution implementation..."
ast-grep --pattern 'handleBridgeHook($ctx, $data, $gas)'
# Check test coverage for different gas scenarios
echo "Checking test coverage for gas scenarios..."
rg -A 10 "Test.*Hook.*Gas" 🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 441-442: x/opchild/keeper/msg_server.go#L441-L442
Added lines #L441 - L442 were not covered by tests
439-443: Add test coverage for params retrieval error handling.
The error handling for params retrieval is not covered by tests. Consider adding test cases to verify the behavior when params retrieval fails.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 441-442: x/opchild/keeper/msg_server.go#L441-L442
Added lines #L441 - L442 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
x/opchild/keeper/deposit.go (4)
13-16: Enhance the error message for better debugging.While the validation is good, consider making the error message more descriptive to aid in debugging.
- return false, "hook max gas is zero" + return false, "invalid hook_max_gas: value must be greater than zero"
18-22: Consider adding minimum gas validation.While the code correctly caps the maximum gas, it doesn't ensure there's enough gas for minimal hook execution. Consider adding a minimum gas threshold check.
if hookMaxGas == 0 { return false, "hook max gas is zero" } + + // Ensure minimum gas for hook execution + const minHookGas uint64 = 20_000 // adjust based on minimal hook requirements + if hookMaxGas < minHookGas { + return false, fmt.Sprintf("hook_max_gas too low: minimum required is %d", minHookGas) + }
37-38: Enhance the comment for better context.While the implementation is correct, consider making the comment more descriptive to explain the purpose of the gas meter replacement.
- // use new gas meter with the hook max gas limit + // Replace gas meter to enforce hook_max_gas limit and prevent excessive gas consumption during hook execution
Line range hint
13-39: Consider monitoring and tuning gas parameters.The gas management implementation is solid, but consider:
- Adding metrics to monitor hook gas usage in production
- Making the minimum gas requirement configurable via chain parameters
- Documenting recommended gas values in the chain documentation
This will help operators tune these parameters based on real-world usage patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/opchild/keeper/deposit.go(2 hunks)
🔇 Additional comments (2)
x/opchild/keeper/deposit.go (2)
7-9: LGTM: Import changes are appropriate.
The addition of storetypes import is necessary for the new gas management functionality.
33-34: LGTM: Gas consumption in defer block is handled correctly.
The implementation properly accounts for all gas consumed during hook execution and correctly attributes it to "bridge hook".
Description
This pull request introduces a new parameter
hook_max_gasfor theMsgFinalizeTokenDepositmessage and to prevent deposit tx rejection due to gas limit.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
hook_max_gasto specify maximum gas for hook execution.FinalizeTokenDepositmethod to include gas handling for bridge hooks and improved event emission based on operation success.hook_max_gasto streamline parameter setup.Bug Fixes
Tests