Skip to content
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

Lo6165 price bump cap #62

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,16 @@ func NewETHClient(endpoint string, opts ...Option) (*ETHClient, error) {
if err != nil {
return nil, err
}
return NewETHClientWith(ethclient.NewClient(rpcClient), opts...)
}

func NewETHClientWith(cli *ethclient.Client, opts ...Option) (*ETHClient, error) {
opt := DefaultOption()
for _, o := range opts {
o(opt)
}
return &ETHClient{
Client: ethclient.NewClient(rpcClient),
Client: cli,
option: *opt,
}, nil
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/client/txpool/txpool.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,12 @@ func inclByPercent(n *big.Int, percent uint64) {
}

// GetMinimumRequiredFee returns the minimum fee required to successfully send a transaction
func GetMinimumRequiredFee(ctx context.Context, client *ethclient.Client, address common.Address, nonce uint64, priceBump uint64) (*big.Int, *big.Int, error) {
func GetMinimumRequiredFee(ctx context.Context, client *ethclient.Client, address common.Address, nonce uint64, priceBump uint64) (*RPCTransaction, *big.Int, *big.Int, error) {
pendingTxs, err := PendingTransactions(ctx, client, address)
if err != nil {
return nil, nil, err
return nil, nil, nil, err
} else if len(pendingTxs) == 0 {
return common.Big0, common.Big0, nil
return nil, common.Big0, common.Big0, nil
}

var targetTx *RPCTransaction
Expand All @@ -98,14 +98,14 @@ func GetMinimumRequiredFee(ctx context.Context, client *ethclient.Client, addres
}
}
if targetTx == nil {
return common.Big0, common.Big0, nil
return nil, common.Big0, common.Big0, nil
}

gasFeeCap := targetTx.GasFeeCap.ToInt()
gasTipCap := targetTx.GasTipCap.ToInt()
gasFeeCap := new(big.Int).Set(targetTx.GasFeeCap.ToInt())
gasTipCap := new(big.Int).Set(targetTx.GasTipCap.ToInt())
Copy link
Contributor Author

@dai1975 dai1975 Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hexuti.Big.ToInt() returns same pointer of receiver instance and following inclByPercent() overwrite it. This make problems when targetTx object is stored and reused such as testing mock.


inclByPercent(gasFeeCap, priceBump)
inclByPercent(gasTipCap, priceBump)

return gasFeeCap, gasTipCap, nil
return targetTx, gasFeeCap, gasTipCap, nil
}
4 changes: 3 additions & 1 deletion pkg/client/txpool/txpool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,10 @@ func transfer(t *testing.T, ctx context.Context, client *ethclient.Client, signe
func replace(t *testing.T, ctx context.Context, client *ethclient.Client, signer types.Signer, key *ecdsa.PrivateKey, priceBump uint64, nonce uint64, gasTipCap, gasFeeCap *big.Int, to common.Address, amount *big.Int) {
addr := crypto.PubkeyToAddress(key.PublicKey)

if minFeeCap, minTipCap, err := GetMinimumRequiredFee(ctx, client, addr, nonce, priceBump); err != nil {
if tx, minFeeCap, minTipCap, err := GetMinimumRequiredFee(ctx, client, addr, nonce, priceBump); err != nil {
t.Fatalf("failed to get the minimum fee required to replace tx: err=%v", err)
} else if tx == nil {
t.Fatalf("replacing tx is not found")
} else if minFeeCap.Cmp(common.Big0) == 0 {
t.Fatalf("tx to replace not found")
} else {
Expand Down
6 changes: 3 additions & 3 deletions pkg/relay/ethereum/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ type Chain struct {
codec codec.ProtoCodecMarshaler
msgEventListener core.MsgEventListener

client *client.ETHClient
client *ChainClient // TODO: use IChainClient after defining all methods in the interface
ibcHandler *ibchandler.Ibchandler
multicall3 *multicall3.Multicall3

Expand Down Expand Up @@ -117,7 +117,7 @@ func NewChain(config ChainConfig) (*Chain, error) {

return &Chain{
config: config,
client: client,
client: &ChainClient{ ETHClient: client },
chainID: id,

ibcHandler: ibcHandler,
Expand Down Expand Up @@ -192,7 +192,7 @@ func (c *Chain) Codec() codec.ProtoCodecMarshaler {

// Client returns the RPC client for ethereum
func (c *Chain) Client() *client.ETHClient {
return c.client
return c.client.ETHClient
}

// SetRelayInfo sets source's path and counterparty's info to the chain
Expand Down
30 changes: 26 additions & 4 deletions pkg/relay/ethereum/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@ import (
"context"
"math/big"

"github.com/ethereum/go-ethereum"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/accounts/abi/bind"

"github.com/datachainlab/ethereum-ibc-relay-chain/pkg/client"
"github.com/datachainlab/ethereum-ibc-relay-chain/pkg/client/txpool"
)

func (chain *Chain) CallOpts(ctx context.Context, height int64) *bind.CallOpts {
Expand All @@ -26,10 +31,6 @@ func (chain *Chain) TxOpts(ctx context.Context, useLatestNonce bool) (*bind.Tran
Signer: chain.ethereumSigner.Sign,
}

if err := NewGasFeeCalculator(chain.client, &chain.config).Apply(ctx, txOpts); err != nil {
return nil, err
}

if useLatestNonce {
if nonce, err := chain.client.NonceAt(ctx, addr, nil); err != nil {
return nil, err
Expand All @@ -38,5 +39,26 @@ func (chain *Chain) TxOpts(ctx context.Context, useLatestNonce bool) (*bind.Tran
}
}

if err := NewGasFeeCalculator(chain.client, &chain.config).Apply(ctx, txOpts); err != nil {
return nil, err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

statement move to fix bug

return txOpts, nil
}

// wrapping interface of client.ETHClient struct
type IChainClient interface {
ethereum.ChainReader
ethereum.GasPricer
ethereum.FeeHistoryReader

GetMinimumRequiredFee(ctx context.Context, address common.Address, nonce uint64, priceBump uint64) (*txpool.RPCTransaction, *big.Int, *big.Int, error);
}

type ChainClient struct {
*client.ETHClient
}

func (cl *ChainClient) GetMinimumRequiredFee(ctx context.Context, address common.Address, nonce uint64, priceBump uint64) (*txpool.RPCTransaction, *big.Int, *big.Int, error) {
return txpool.GetMinimumRequiredFee(ctx, cl.ETHClient.Client, address, nonce, priceBump);
}
18 changes: 13 additions & 5 deletions pkg/relay/ethereum/gas.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,18 @@ import (
"fmt"
"math/big"

"github.com/datachainlab/ethereum-ibc-relay-chain/pkg/client"
"github.com/datachainlab/ethereum-ibc-relay-chain/pkg/client/txpool"
"github.com/ethereum/go-ethereum"
"github.com/ethereum/go-ethereum/accounts/abi/bind"
"github.com/ethereum/go-ethereum/common"
)

type GasFeeCalculator struct {
client *client.ETHClient
client IChainClient
config *ChainConfig
}

func NewGasFeeCalculator(client *client.ETHClient, config *ChainConfig) *GasFeeCalculator {
func NewGasFeeCalculator(client IChainClient, config *ChainConfig) *GasFeeCalculator {
return &GasFeeCalculator{
client: client,
config: config,
Expand All @@ -27,19 +26,22 @@ func NewGasFeeCalculator(client *client.ETHClient, config *ChainConfig) *GasFeeC
func (m *GasFeeCalculator) Apply(ctx context.Context, txOpts *bind.TransactOpts) error {
minFeeCap := common.Big0
minTipCap := common.Big0
var oldTx *txpool.RPCTransaction
if m.config.PriceBump > 0 && txOpts.Nonce != nil {
var err error
if minFeeCap, minTipCap, err = txpool.GetMinimumRequiredFee(ctx, m.client.Client, txOpts.From, txOpts.Nonce.Uint64(), m.config.PriceBump); err != nil {
if oldTx, minFeeCap, minTipCap, err = m.client.GetMinimumRequiredFee(ctx, txOpts.From, txOpts.Nonce.Uint64(), m.config.PriceBump); err != nil {
return err
}
}

switch m.config.TxType {
case TxTypeLegacy:
gasPrice, err := m.client.SuggestGasPrice(ctx)
if err != nil {
return fmt.Errorf("failed to suggest gas price: %v", err)
}
if oldTx != nil && oldTx.GasPrice != nil && oldTx.GasPrice.ToInt().Cmp(gasPrice) > 0 {
return fmt.Errorf("old tx's gasPrice(%v) is higher than suggestion(%v)", oldTx.GasPrice.ToInt(), gasPrice)
}
if gasPrice.Cmp(minFeeCap) < 0 {
gasPrice = minFeeCap
}
Expand All @@ -52,6 +54,9 @@ func (m *GasFeeCalculator) Apply(ctx context.Context, txOpts *bind.TransactOpts)
}
// GasTipCap = min(LimitPriorityFeePerGas, simulated_eth_maxPriorityFeePerGas * PriorityFeeRate)
m.config.DynamicTxGasConfig.PriorityFeeRate.Mul(gasTipCap)
if oldTx != nil && oldTx.GasTipCap != nil && oldTx.GasTipCap.ToInt().Cmp(gasTipCap) > 0 {
return fmt.Errorf("old tx's gasTipCap(%v) is higher than suggestion(%v)", oldTx.GasTipCap.ToInt(), gasTipCap)
}
if gasTipCap.Cmp(minTipCap) < 0 {
gasTipCap = minTipCap
}
Expand All @@ -61,6 +66,9 @@ func (m *GasFeeCalculator) Apply(ctx context.Context, txOpts *bind.TransactOpts)
// GasFeeCap = min(LimitFeePerGas, GasTipCap + BaseFee * BaseFeeRate)
m.config.DynamicTxGasConfig.BaseFeeRate.Mul(gasFeeCap)
gasFeeCap.Add(gasFeeCap, gasTipCap)
if oldTx != nil && oldTx.GasFeeCap != nil && oldTx.GasFeeCap.ToInt().Cmp(gasFeeCap) > 0 {
return fmt.Errorf("old tx's gasFeeCap(%v) is higher than suggestion(%v)", oldTx.GasFeeCap.ToInt(), gasFeeCap)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dai1975 I think that we should give up tx replacement only if both the tip and fee caps of the pending tx are higher than the new tx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely. fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add error case when new caps are equals to old caps(Cmp returns zero)

if gasFeeCap.Cmp(minFeeCap) < 0 {
gasFeeCap = minFeeCap
}
Expand Down
Loading