fix: hardcode gas limits to avoid estimate gas calls#3602
Conversation
skosito
added
ADMIN_TESTS
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces hardcoded gas limits to replace dynamic gas estimation. A new changelog entry documents the change for pull request [3602]. Updates span multiple files, including modifications to the compiled contract binaries and adjustments in the TestGasConsumer contract, as well as changes in various keeper modules where gas limits, gas prices, and EVM call parameters are updated from dynamic values (or Changes
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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3602 +/- ##
========================================
Coverage 64.69% 64.69%
========================================
Files 466 466
Lines 32477 32477
========================================
Hits 21010 21010
Misses 10512 10512
Partials 955 955
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/fungible/keeper/zrc20_methods.go (1)
160-160: Ensure consistency for read-only operation gas handlingThese changes follow the same pattern: setting gas limit to
niland changingtruetofalsefor the commit parameter in read-only operations. While this differs from the PR's overall approach of hardcoding gas limits, it may be an optimization for view functions.Consider adding a comment explaining this pattern difference, as it's not immediately obvious why read-only operations use a different approach than write operations.
Also applies to: 218-218, 274-274, 326-326
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
e2e/contracts/testgasconsumer/TestGasConsumer.binis excluded by!**/*.bin
📒 Files selected for processing (11)
changelog.md(1 hunks)e2e/contracts/testgasconsumer/TestGasConsumer.go(1 hunks)e2e/contracts/testgasconsumer/TestGasConsumer.json(1 hunks)e2e/contracts/testgasconsumer/TestGasConsumer.sol(1 hunks)x/fungible/keeper/evm.go(4 hunks)x/fungible/keeper/evm_gateway.go(1 hunks)x/fungible/keeper/gas_coin_and_pool.go(2 hunks)x/fungible/keeper/gas_price.go(3 hunks)x/fungible/keeper/msg_server_update_system_contract.go(3 hunks)x/fungible/keeper/system_contract.go(2 hunks)x/fungible/keeper/zrc20_methods.go(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/contracts/testgasconsumer/TestGasConsumer.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/msg_server_update_system_contract.gox/fungible/keeper/evm_gateway.gox/fungible/keeper/gas_coin_and_pool.gox/fungible/keeper/gas_price.gox/fungible/keeper/zrc20_methods.gox/fungible/keeper/evm.gox/fungible/keeper/system_contract.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-performance-test / e2e
🔇 Additional comments (22)
x/fungible/keeper/evm_gateway.go (1)
19-19: Increased gateway gas limit to accommodate larger transactions.The increase from 1,000,000 to 1,500,000 gas units addresses the issue where certain
depositAndCalloperations were failing due to insufficient gas limits. This conservative adjustment ensures proper execution of complex cross-chain transactions while avoiding dynamic gas estimation.changelog.md (1)
38-38: Appropriate changelog entry for gas limit hardcoding.The entry properly documents the change made in PR #3602, providing clear context about the nature of the fix (hardcoding gas limits to avoid estimate gas calls). This maintains good transparency about system modifications.
e2e/contracts/testgasconsumer/TestGasConsumer.json (1)
49-49: Updated binary to reflect the reduced gas consumption target.The updated contract binary reflects the compilation of the modified Solidity source code where target gas consumption was reduced. This binary change is expected and necessary to maintain synchronization between source and compiled code.
e2e/contracts/testgasconsumer/TestGasConsumer.sol (1)
29-29: Reduced target gas consumption to align with system gas limits.The target gas consumption has been reduced from 5,000,000 to 1,500,000, bringing it in line with the gateway gas limit set in the system. This adjustment provides a more realistic test scenario and better visibility into gas limit behavior at the node level.
x/fungible/keeper/gas_price.go (3)
37-37: Approve hardcoded gas limit for SetGasPriceThe change from a dynamic gas estimate to a fixed value aligns with the PR objective to avoid excessive gas consumption during estimation. The 100,000 gas limit for this setter function should be sufficient for the operation.
73-73: Approve hardcoded gas limit for SetGasCoinSetting a fixed gas limit of 100,000 for this setter function is appropriate and maintains consistency with other similar operations in the codebase.
109-109: Approve increased gas limit for SetGasZetaPoolThe higher gas limit of 200,000 for this function is appropriate as it likely involves more complex operations than simple setters. This aligns with the conservative approach mentioned in the PR objectives.
x/fungible/keeper/gas_coin_and_pool.go (2)
114-114: Approve hardcoded gas limit for setGasZetaPoolSetting a fixed gas limit of 200,000 for the setGasZetaPool operation aligns with the more complex setter pattern established in this PR. The comment on line 22 "FIXME: add cointype and use proper gas limit based on cointype/chain" remains relevant but isn't directly affected by this change.
149-149: Approve hardcoded gas limit for approveThe 200,000 gas limit for the approve operation is consistent with other complex operations in this PR and should be sufficient for token approval interactions.
x/fungible/keeper/msg_server_update_system_contract.go (3)
61-61: Approve hardcoded gas limit for updateSystemContractAddressThe 100,000 gas limit is appropriate for this system contract update function and aligns with the pattern of using 100,000 for simple setter operations.
81-81: Approve hardcoded gas limit for setGasCoinZRC20Using a 100,000 gas limit for this setter function maintains consistency with other similar operations in the PR.
101-101: Approve hardcoded gas limit for setGasZetaPoolThe 200,000 gas limit is consistent with other instances of setGasZetaPool in the codebase, maintaining a uniform approach to gas allocation for this operation.
x/fungible/keeper/zrc20_methods.go (4)
46-46: Approve gas limit reduction for ZRC20SetNameReducing the gas limit to 100,000 for this setter function is reasonable and consistent with other setters in the codebase.
80-80: Approve gas limit reduction for ZRC20SetSymbolThe reduced gas limit of 100,000 for this setter function aligns with the pattern established in this PR.
382-382: Approve hardcoded gas limits for transfer operationsSetting a fixed gas limit of 200,000 for transfer operations is appropriate and consistent with the PR's approach for more complex operations. These transfer functions likely require more gas than simple setters but should be well within the 200,000 limit for typical cases.
Also applies to: 442-442
113-113:Details
✅ Verification successful
Verify impact of changing gas limit to nil for read-only method
Changing from a hardcoded value to
nilfor this read-only method seems inconsistent with the PR's objective of avoiding estimate gas calls. This might be intentional for read-only operations, but warrants verification.
🏁 Script executed:
#!/bin/bash # Check if there are other read-only methods in the codebase using nil gas limits echo "Searching for patterns of nil gas limits in read-only methods..." rg -B2 -A10 "CallEVM\(" --type go | grep -B5 -A5 "nil," | grep -A2 "function.*view" || echo "No direct matches found" # Check if there are any comments or documentation explaining this pattern echo "Searching for documentation about gas limit patterns..." rg -A3 "gas limit" --type go | grep -i "read-only\|view\|query" || echo "No explicit documentation found"Length of output: 2225
Subject: Nil Gas Limit for Read-Only Method Verified
After reviewing the codebase and running the verification scripts, there are no conflicting patterns or documentation that suggest using
nilfor the gas limit in read-only methods is problematic. In fact, the absence of a hardcoded gas limit aligns with the PR’s objective to avoid unnecessary gas estimation on view methods. Although similar patterns were not found elsewhere in read-only implementations, tests and other references confirm that explicit gas limit queries are generally reserved for transactional (non-read-only) operations.Recommendations:
- Maintain the use of
nilfor the read-only gas limit in this context.- Consider adding a concise code comment explaining that
nilis used because no gas estimation is required for read-only operations, which will aid future maintainers.x/fungible/keeper/system_contract.go (2)
377-377: Consistent numeric literal formatting.The change from
big.NewInt(1000_000)tobig.NewInt(1_000_000)improves readability by using a consistent thousands separator format throughout the codebase.
800-800: Hardcoded gas limit replaces nil value.This change aligns with the PR objective of hardcoding gas limits to avoid estimate gas calls. The value of 200,000 is appropriate for a ZRC20 approval operation, which is a moderately complex setter.
x/fungible/keeper/evm.go (4)
43-44: Increased gas limits for deposit and connector calls.The gas limits for
ZEVMGasLimitDepositAndCallandZEVMGasLimitConnectorCallhave been increased from 1,000,000 to 1,500,000. This addresses issue #3592 where certaindepositAndCalloperations were hitting the previous gas limit.
261-261: Hardcoded gas limit for ZRC20 deposit operations.Replacing the
nilgas limit with a hardcoded value of 200,000 is appropriate for deposit operations, which are more complex than simple setters but less complex than full deposit-and-call operations.
286-286: Hardcoded gas limit for protocol flat fee updates.The gas limit of 100,000 is suitable for this simple setter operation, aligning with the PR objective of using conservative hardcoded gas limits.
310-310: Hardcoded gas limit for gas limit updates.Similar to the protocol flat fee update, this simple setter operation receives a conservative hardcoded gas limit of 100,000, which is appropriate for its complexity level.
3 participants
Description
if gas limit is nil in
CallEVM, it will be estimated, which takes a lot of cosmos gasgas limits are hardcoded in following conservative way:
also, for some
depositAndCallcalls, 1M is not enough, so bumping gateway limit to 1.5M (check linked issues) and fixing gas consumer test to keep that limit incloses: #3592, #3537
How Has This Been Tested?
Summary by CodeRabbit
Documentation
Chores