feat(cmd/mito): implement dynamic gas estimation for transactions #85
Conversation
- Enhanced the transaction builder to estimate gas limits dynamically based on the transaction data and signer address. - Updated the `CreateTransactionFromDataWithOptions` method to include gas estimation logic, adding a 20% buffer to the estimated gas. - Modified collateral and validator services to set gas limits to zero, allowing the builder to handle gas estimation, improving transaction efficiency and flexibility. These changes optimize gas usage for transactions, ensuring more accurate gas limits and reducing the likelihood of transaction failures due to insufficient gas.
There was a problem hiding this comment.
Pull Request Overview
This PR implements dynamic gas estimation for transactions to replace hardcoded gas limits with estimated values based on transaction data. The changes remove manual gas limit calculations from validator and collateral services, delegating gas estimation to the transaction builder with a 20% buffer.
- Removed hardcoded gas limit logic from validator and collateral services
- Added dynamic gas estimation in the transaction builder with RPC client integration
- Updated default gas limits to be more conservative when estimation is unavailable
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/mito/internal/tx/validator.go | Removed gas limit calculation logic, setting GasLimit to 0 for builder estimation |
| cmd/mito/internal/tx/collateral.go | Removed gas limit calculation logic, setting GasLimit to 0 for builder estimation |
| cmd/mito/internal/tx/builder.go | Added dynamic gas estimation with RPC client and 20% buffer calculation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| fromAddr, err := b.GetSignerAddress() | ||
| if err != nil { | ||
| // Fallback to default if we can't get signer address | ||
| gasLimit = 200000 |
There was a problem hiding this comment.
The fallback gas limit of 200000 is significantly lower than the previous default of 500000. This could cause transaction failures for operations that require more gas. Consider using a more conservative fallback value or documenting why this reduction is safe.
| gasLimit = 200000 | |
| gasLimit = 500000 |
| } | ||
| } | ||
| } else { | ||
| gasLimit = 200000 // Conservative default when no RPC available or unsigned transaction |
There was a problem hiding this comment.
The comment describes this as 'Conservative default' but 200000 is actually less conservative than the previous 500000 default. This inconsistency between the comment and the actual behavior could be misleading.
| gasLimit = 200000 // Conservative default when no RPC available or unsigned transaction | |
| gasLimit = 500000 // Conservative default when no RPC available or unsigned transaction |
| return nil, fmt.Errorf("gas estimation failed: %w", err) | ||
| } else { | ||
| // Add 20% buffer to estimated gas | ||
| gasLimit = estimatedGas + (estimatedGas / 5) |
There was a problem hiding this comment.
The gas buffer calculation uses integer division which could result in precision loss for small gas estimates. Consider using gasLimit = (estimatedGas * 6) / 5 to avoid potential rounding issues.
| gasLimit = estimatedGas + (estimatedGas / 5) | |
| gasLimit = (estimatedGas * 6) / 5 |
|
CreateTransactionFromDataWithOptionsmethod to include gas estimation logic, adding a 20% buffer to the estimated gas.These changes optimize gas usage for transactions, ensuring more accurate gas limits and reducing the likelihood of transaction failures due to insufficient gas.