Consolidate the validations for max TX gas limit #759
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
WalkthroughThis update modifies error handling and gas limit validation across multiple components. API methods now return errors directly without additional wrapping, and references to the deprecated gas limit constant have been replaced with a new constant defined in the models package. The changes also include stricter gas limit validation in transaction validation logic, along with corresponding test cases to ensure that transactions exceeding the maximum gas limit are rejected. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as API Handler
participant TX as Transaction Validator
C->>API: Submit Transaction Request
API->>TX: Validate Transaction Gas Limit (using TxMaxGasLimit)
alt Gas Limit Valid
TX-->>API: Return success
API-->>C: Process Transaction
else Gas Limit Exceeded
TX-->>API: Return error (Gas limit exceeded)
API-->>C: Error Response
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 0
🧹 Nitpick comments (1)
api/api.go (1)
730-732: Consider error propagation in EstimateGas.Instead of returning the max gas limit on encoding error, consider propagating the error to provide better feedback to the caller.
- // return max tx gas limit - return hexutil.Uint64(models.TxMaxGasLimit), nil + return 0, fmt.Errorf("failed to encode transaction: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/api.go(3 hunks)api/debug.go(1 hunks)api/utils.go(2 hunks)models/transaction.go(3 hunks)models/transaction_test.go(1 hunks)services/requester/requester.go(2 hunks)tests/web3js/debug_traces_test.js(1 hunks)tests/web3js/eth_failure_handling_test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (14)
api/utils.go (2)
12-12: LGTM!The import is correctly added to access the
TxMaxGasLimitconstant from the models package.
157-157: LGTM!The gas limit is correctly updated to use the standardized
models.TxMaxGasLimitconstant, aligning with the PR objectives.tests/web3js/eth_failure_handling_test.js (3)
6-27: LGTM!The test case correctly validates that transactions with gas limit exceeding 50M are rejected with the appropriate error message.
29-49: LGTM!The test case correctly validates that eth_call operations with gas limit exceeding 50M are rejected with the appropriate error message.
51-71: LGTM!The test case correctly validates that eth_estimateGas operations with gas limit exceeding 50M are rejected with the appropriate error message.
models/transaction.go (2)
10-10: LGTM!The import is correctly added to access error types for gas limit validation.
32-35: LGTM!The implementation correctly:
- Defines the maximum gas limit constant with clear documentation.
- Adds gas limit validation as an early check in the transaction validation flow.
- Uses appropriate error handling for validation failures.
Also applies to: 276-278
api/debug.go (1)
197-197: LGTM!The gas limit is correctly updated to use the standardized
models.TxMaxGasLimitconstant, maintaining consistency across the codebase.services/requester/requester.go (2)
331-332: LGTM! Gas limit standardization in EstimateGas.The code now uses
models.TxMaxGasLimitas the upper bound for gas estimation, aligning with the standardization of gas limits across endpoints.
471-472: LGTM! Gas limit standardization in getBlockView.The code now uses
models.TxMaxGasLimitfor view provider initialization, maintaining consistency in gas limit usage.models/transaction_test.go (1)
306-319: LGTM! Comprehensive test case for gas limit validation.The test case verifies that transactions with gas limits exceeding
TxMaxGasLimitare correctly rejected, with appropriate error messaging.tests/web3js/debug_traces_test.js (1)
660-679: LGTM! Integration test for gas limit validation.The test verifies that
debug_traceCallcorrectly rejects transactions with gas limits exceeding 50M, ensuring consistent gas limit validation across endpoints.api/api.go (2)
581-582: LGTM! Simplified error handling in Call method.Direct error propagation improves error transparency.
751-752: LGTM! Simplified error handling in EstimateGas.Direct error propagation improves error transparency.
m-Peter
force-pushed
the
mpeter/consolidate-tx-max-gas-limit
branch
from
dd7b851 to
1848004
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
services/requester/requester.go (1)
46-46: 🛠️ Refactor suggestionRemove unused constant and update header initialization.
The
blockGasLimitconstant is no longer used after standardizing gas limits. Update the code as follows:-const blockGasLimit = 120_000_000 ... - GasLimit: blockGasLimit, + GasLimit: models.TxMaxGasLimit,Also applies to: 149-149
🧹 Nitpick comments (2)
models/transaction.go (1)
32-35: Clarify the relationship between computation limit and gas limit.The comment mentions "9999 as the computation limit" but doesn't explain how this relates to the 50M gas limit value. Consider expanding the comment to explain this relationship for better maintainability.
tests/web3js/eth_failure_handling_test.js (1)
29-49: Consider standardizing error messages across endpoints.The error message format differs from the
eth_sendRawTransactionendpoint:
eth_sendRawTransaction: "tx gas limit exceeds the max value of 50000000"eth_call: "gas limit is bigger than max gas limit allowed 51000000 > 50000000"Consider using consistent error message formats across all endpoints for better user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/api.go(3 hunks)api/debug.go(1 hunks)api/utils.go(2 hunks)models/transaction.go(3 hunks)models/transaction_test.go(1 hunks)services/requester/requester.go(2 hunks)tests/web3js/debug_traces_test.js(1 hunks)tests/web3js/eth_failure_handling_test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- api/debug.go
- api/utils.go
- tests/web3js/debug_traces_test.js
- models/transaction_test.go
- api/api.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (6)
services/requester/requester.go (3)
191-193: LGTM! Centralized validation improves consistency.The change to use
models.ValidateTransactioncentralizes the gas limit validation, which aligns with the PR objective to standardize validation across endpoints.
331-331: LGTM! Standardized gas limit in estimation logic.Using
models.TxMaxGasLimitensures consistency with other endpoints while maintaining the efficient binary search algorithm for gas estimation.
471-471: LGTM! Standardized gas limit in view provider.Using
models.TxMaxGasLimitin the view provider ensures consistent gas limits foreth_callanddebug_traceCallendpoints.models/transaction.go (1)
276-278: LGTM! Clean and effective gas limit validation.The implementation correctly validates the transaction gas limit against the new maximum value, with clear error handling.
tests/web3js/eth_failure_handling_test.js (2)
6-27: LGTM! Comprehensive test for raw transaction gas limit.The test effectively verifies that transactions exceeding the 50M gas limit are rejected with the appropriate error message.
51-71: LGTM! Good test coverage for estimateGas endpoint.The test effectively verifies gas limit validation for the
eth_estimateGasendpoint, maintaining consistency with theeth_calltest structure.
| @@ -189,10 +188,6 @@ func (e *EVM) SendRawTransaction(ctx context.Context, data []byte) (common.Hash, | |||
| return common.Hash{}, err | |||
| } | |||
|
|
|||
| if tx.Gas() > txMaxGasLimit { | |||
There was a problem hiding this comment.
Why is this check removed?
There was a problem hiding this comment.
It was simply moved to https://github.com/onflow/flow-evm-gateway/pull/759/files#diff-eda2e152959416cf6c769f6704b883b0e42711ba10a3d0792e6d13748f515fe0R276-R279. Where the rest of the validation logic happens.
Closes: #758
Description
The max TX gas limit for
eth_sendRawTransactionwas recently set to50M.Now we apply this same limit to the following endpoints:
eth_calleth_estimateGasdebug_traceCallFor contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Refactor
Tests