-
Notifications
You must be signed in to change notification settings - Fork 16
Add support for price bump when tx_type="auto" #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Takeshi Arabiki <takeshi.arabiki@datachain.jp>
Signed-off-by: Takeshi Arabiki <takeshi.arabiki@datachain.jp>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for the price_bump parameter when tx_type is set to "auto". Previously, price bumping was only supported for "legacy" and "dynamic" transaction types.
Key changes:
- Implemented price bump logic for TxTypeAuto in both legacy (pre-EIP-1559) and EIP-1559 modes
- Refactored gas price calculation into a reusable
calculateGasPricehelper function - Added
ethereum.GasPricer1559interface for EIP-1559 gas price suggestions - Enhanced test coverage with new test functions for auto transaction type with price bumping
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/relay/ethereum/gas.go | Implements TxTypeAuto case with price bump support, refactors common gas price calculation logic into calculateGasPrice helper, changes default case to panic for unsupported tx types |
| pkg/relay/ethereum/gas_test.go | Adds test cases for price bumping with auto tx type (both legacy and EIP-1559 modes), updates MockChainClient to support EIP-1559 suggestions, fixes test comments and adds defensive copies for big.Int values |
| pkg/relay/ethereum/client.go | Extends IChainClient interface to include ethereum.GasPricer1559 for EIP-1559 gas price suggestions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if gasFeeCap.Cmp(gasTipCap) < 0 { | ||
| return fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", gasFeeCap, gasTipCap) | ||
| } |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation to check if the old transaction's gas values already exceed the suggestions. The TxTypeDynamic case includes this check (lines 58-62):
if oldTx != nil && oldTx.GasFeeCap != nil && oldTx.GasTipCap != nil {
if oldTx.GasFeeCap.ToInt().Cmp(gasFeeCap) >= 0 && oldTx.GasTipCap.ToInt().Cmp(gasTipCap) >= 0 {
return fmt.Errorf("old tx's gasFeeCap(%v) and gasTipCap(%v) are greater than or equal to suggestion(%v, %v)", ...)
}
}This validation should be added before line 118 to ensure that when both old tx values are already at or above the suggested values, an error is returned instead of attempting to use potentially stale values.
| } | |
| } | |
| if oldTx != nil && oldTx.GasFeeCap != nil && oldTx.GasTipCap != nil { | |
| if oldTx.GasFeeCap.ToInt().Cmp(gasFeeCap) >= 0 && oldTx.GasTipCap.ToInt().Cmp(gasTipCap) >= 0 { | |
| return fmt.Errorf("old tx's gasFeeCap(%v) and gasTipCap(%v) are greater than or equal to suggestion(%v, %v)", oldTx.GasFeeCap.ToInt(), oldTx.GasTipCap.ToInt(), gasFeeCap, gasTipCap) | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the validation in the commit 5c4ff80
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Takeshi Arabiki <takeshi.arabiki@datachain.jp>
Although the PR #59 introduced the
price_bumpparameter, it is currently applied only when thetx_typeis set to "legacy" or "dynamic".This PR adds support for the
price_bumpparameter whentx_typeis "auto" as well.I've confirmed that the CI in the cosmos-ethereum-ibc-lcp passes.
datachainlab/cosmos-ethereum-ibc-lcp#88