Lo6165 price bump cap by dai1975 · Pull Request #62 · datachainlab/ethereum-ibc-relay-chain

dai1975 · 2025-01-30T09:49:45Z

fix price bump bug to set opts.Nonce before GasFeeCalculator#Apply
check latest(replacing) transaction's gas fee is already high enough
make IETHClient interface and use it in gas.go and txpool.go for testing

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

pkg/client/client.go

dai1975 · 2025-01-30T09:54:48Z

pkg/client/txpool/txpool.go

-	gasFeeCap := targetTx.GasFeeCap.ToInt()
-	gasTipCap := targetTx.GasTipCap.ToInt()
+	gasFeeCap := new(big.Int).Set(targetTx.GasFeeCap.ToInt())
+	gasTipCap := new(big.Int).Set(targetTx.GasTipCap.ToInt())


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.

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

siburu · 2025-02-03T11:00:08Z

@dai1975 You have replaced the ETHClient struct with the IETHClient interface to mock it for testing. They are all defined in the pkg/client package.
However, actual tests that want to mock the ethereum client are not in pkg/client but in pkg/relay/ethereum (gas_test.go).
Therefore, I think, you didn't need to modify the pkg/client package and should only have modified the type of the client argument of NewGasFeeCalculator.

siburu · 2025-02-03T11:05:13Z

@dai1975 To easily mock all the functions of the pkg/client package and the pkg/client/txpool package, you may need to move the functions defined in the pkg/client/txpool package to the pkg/client and convert them into methods of the ETHClient struct.

Specifically, GetMinimumRequiredFee must be converted into a method of the ETHClient, since it is called from GasFeeCalculator.
However, I suggest to convert all the other functions (ContentFrom and PendingTransactions), too.

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

dai1975 · 2025-02-05T08:48:47Z

pkg/relay/ethereum/client.go

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


statement move to fix bug

dai1975 · 2025-02-05T08:51:16Z

@siburu moved interface definition to ethereum package.

pkg/relay/ethereum/client.go

pkg/relay/ethereum/gas.go

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

pkg/relay/ethereum/gas.go

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

siburu

@dai1975 Sorry but I have specified some to fix. Please check them 🙇

siburu · 2025-02-13T07:34:19Z

pkg/relay/ethereum/gas.go

+
+		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 higher than suggestion(%v, %v)", oldTx.GasFeeCap.ToInt(), oldTx.GasTipCap.ToInt(), gasFeeCap, gasTipCap)


Please fix the error message by replacing higher than with greater than or equal to.

siburu · 2025-02-13T07:35:09Z

pkg/relay/ethereum/gas_test.go

+		cli.MockHistoryGasTipCap.SetUint64(99) // lower than 100
+		cli.MockHistoryGasFeeCap.SetUint64(299 - 99) //lower than 300


You should replace 99 and 299 - 99 with 100 and 300 - 100 to test the corner case.

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

siburu

@dai1975 LGTM. Great work, thank you!

Daisuke Kanda added 2 commits January 26, 2025 02:33

fix price bump condition

d412a80

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

impl replace price check and make ethclient be interface for testing

c5db28a

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

dai1975 commented Jan 30, 2025

View reviewed changes

pkg/client/client.go Outdated Show resolved Hide resolved

dai1975 commented Jan 30, 2025

View reviewed changes

dai1975 requested review from siburu and toshihiko-okubo January 30, 2025 09:55

define txpool's function as method

6621802

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

Daisuke Kanda added 4 commits February 4, 2025 00:15

move client interface to ethereum package

3213c9c

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

check returned tx in test

a845932

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

remove txpool method from ETHClient

557c2b0

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

use ethereum.* interface

4901d10

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

dai1975 commented Feb 5, 2025

View reviewed changes

siburu reviewed Feb 5, 2025

View reviewed changes

pkg/relay/ethereum/client.go Outdated Show resolved Hide resolved

pkg/relay/ethereum/client.go Show resolved Hide resolved

siburu reviewed Feb 7, 2025

View reviewed changes

pkg/relay/ethereum/gas.go Outdated Show resolved Hide resolved

check both feeCap and tipCap exceed old

19e9c1e

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

siburu reviewed Feb 10, 2025

View reviewed changes

pkg/relay/ethereum/gas.go Outdated Show resolved Hide resolved

check price equility

993e18a

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

siburu reviewed Feb 13, 2025

View reviewed changes

fix test case

1e5dd27

Signed-off-by: Daisuke Kanda <daisuke.kanda@datachain.jp>

siburu approved these changes Feb 13, 2025

View reviewed changes

siburu merged commit 70ce3fd into datachainlab:main Feb 13, 2025
1 check passed

		cli.MockHistoryGasTipCap.SetUint64(99) // lower than 100
		cli.MockHistoryGasFeeCap.SetUint64(299 - 99) //lower than 300

Conversation

dai1975 commented Jan 30, 2025

Uh oh!

Uh oh!

dai1975 Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siburu commented Feb 3, 2025

Uh oh!

siburu commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dai1975 Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

dai1975 commented Feb 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siburu left a comment

Choose a reason for hiding this comment

Uh oh!

siburu Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

dai1975 Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

siburu Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

dai1975 Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

siburu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dai1975 Jan 30, 2025 •

edited

Loading

siburu commented Feb 3, 2025 •

edited

Loading