feat: introduce gas multiplier on jsonrpc endpoint #128
Conversation
WalkthroughThe changes introduce a new 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: 5
🧹 Outside diff range and nitpick comments (4)
jsonrpc/backend/backend.go (1)
Line range hint
1-1: Consider architectural improvementsTo enhance the gas multiplier feature:
- Add documentation explaining the purpose and impact of the gas multiplier
- Consider implementing a centralized configuration validation
- Add metrics/logging for monitoring gas estimation accuracy
- Consider making the multiplier configurable at runtime via an admin API
jsonrpc/config/config.go (3)
45-46: Consider documenting the rationale for the 1.4 multiplier valueWhile 1.4 seems like a reasonable default value for gas estimation buffer, it would be helpful to document why this specific value was chosen. This helps future maintainers understand the reasoning behind this constant.
110-111: Enhance field documentation with validation requirementsThe field documentation should specify:
- Expected format (decimal string)
- Valid range of values
- Impact on gas estimation
Consider updating the comment to:
-// GasMultiplier is the gas multiplier for the EVM state transition. +// GasMultiplier is the gas multiplier for the EVM state transition. +// Must be a valid decimal string greater than 1.0 (e.g. "1.4"). +// This value is used to pad the gas estimation to prevent out-of-gas errors during actual execution.
247-249: Add validation warning in config templateThe gas multiplier configuration in the template should include a warning about validation requirements and potential impacts.
Consider updating the template comment to:
-# GasMultiplier is the gas multiplier for the EVM state transition. +# GasMultiplier is the gas multiplier for the EVM state transition. +# Must be a valid decimal string greater than 1.0 (e.g. "1.4"). +# Warning: Setting this too low may cause transactions to fail with out-of-gas errors, +# while setting it too high will cause users to over-provision gas.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
jsonrpc/backend/backend.go(4 hunks)jsonrpc/backend/gas.go(1 hunks)jsonrpc/backend/gas_test.go(1 hunks)jsonrpc/config/config.go(7 hunks)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #128 +/- ##
==========================================
- Coverage 56.83% 56.81% -0.02%
==========================================
Files 109 109
Lines 9930 9944 +14
==========================================
+ Hits 5644 5650 +6
- Misses 3473 3479 +6
- Partials 813 815 +2
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
jsonrpc/config/config.go (3)
45-46: Document the rationale for the default gas multiplier valueThe default value of 1.4 seems arbitrary. Consider adding a comment explaining why this specific value was chosen.
-// DefaultGasMultiplier is the default gas multiplier for the EVM state transition. +// DefaultGasMultiplier is the default gas multiplier for the EVM state transition. +// The value 1.4 provides a 40% buffer to account for potential gas estimation inaccuracies +// and prevent out-of-gas errors during actual transaction execution. DefaultGasMultiplier = "1.4"
110-111: Add validation constraints to the GasMultiplier field documentationThe field documentation should specify the valid range of values.
-// GasMultiplier is the gas multiplier for the EVM state transition. +// GasMultiplier is the gas multiplier for the EVM state transition. +// Must be a decimal string representing a value >= 1.0 GasMultiplier string `mapstructure:"gas-multiplier"`
247-249: Add validation constraints to the configuration templateThe configuration template should document the valid range for the gas multiplier.
-# GasMultiplier is the gas multiplier for the EVM state transition. +# GasMultiplier is the gas multiplier for the EVM state transition. +# Must be a decimal string representing a value >= 1.0 (e.g., "1.4" for 40% buffer) gas-multiplier = "{{ .JSONRPCConfig.GasMultiplier }}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
jsonrpc/backend/backend.go(4 hunks)jsonrpc/config/config.go(7 hunks)
🔇 Additional comments (3)
jsonrpc/backend/backend.go (3)
131-133: LGTM: Proper initialization of gas multiplier
The initialization of the gas multiplier field in the struct is correct.
56-57:
Add thread safety for gas multiplier access
The gasMultiplier field might be accessed concurrently but lacks synchronization mechanisms.
Add mutex protection:
type JSONRPCBackend struct {
// ... other fields ...
cfg config.JSONRPCConfig
gasMultiplier math.LegacyDec
+ gasMultMutex sync.RWMutex
}Likely invalid or redundant comment.
89-96:
Enhance gas multiplier validation
The gas multiplier validation needs to be strengthened to ensure valid values.
Add validation for the multiplier value:
if cfg.GasMultiplier == "" {
cfg.GasMultiplier = config.DefaultGasMultiplier
}
gasMultiplier, err := math.LegacyNewDecFromStr(cfg.GasMultiplier)
if err != nil {
return nil, err
}
+// Validate multiplier range
+if gasMultiplier.LT(math.LegacyOneDec()) {
+ return nil, fmt.Errorf("gas multiplier must be greater than or equal to 1.0, got %s", cfg.GasMultiplier)
+}Likely invalid or redundant comment.
Description
We have removed the default gas charge for fee deduction during simulation, as we've introduced a gas-free fee payment option when using the JSON-RPC endpoint. However, since Cosmos still applies charges only when the transaction is finalize mode, we've introduced a gas multiplier for the JSON-RPC endpoint to prevent out-of-gas issues.
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
Tests