-
Notifications
You must be signed in to change notification settings - Fork 33
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
+dynamic_gas_unit_add_percentage flag #3494
base: feat/relayer-arb-call
Are you sure you want to change the base?
Changes from 32 commits
dfbf6ba
93459f1
905a3a5
09816fb
63f730c
31ca3cf
d8481f2
d7f1cad
3d7eea2
4c11d33
145583d
2005d91
a8a48bb
75c9c44
96e16c3
dada2b0
7abfdb0
0e7c236
220e0d4
b995385
4782b44
d699cd0
04ccdc2
c8f2dfe
d199850
e8fd10e
ee9eb85
f6bfa43
c79a81b
82b4f99
5b02cca
965802d
7b62df2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -388,13 +388,13 @@ | |||||||||||||||||||||
// this also prevents a bug in the caller from breaking our lock | ||||||||||||||||||||||
transactor.Nonce = new(big.Int).Add(new(big.Int).SetUint64(math.MaxUint64), big.NewInt(1)) | ||||||||||||||||||||||
|
||||||||||||||||||||||
//tmpdebug | ||||||||||||||||||||||
fmt.Printf("SubmitTransaction>setGasPrice\n") | ||||||||||||||||||||||
|
||||||||||||||||||||||
err = t.setGasPrice(ctx, chainClient, transactor, chainID, nil) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
span.AddEvent("could not set gas price", trace.WithAttributes(attribute.String("error", err.Error()))) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) { | ||||||||||||||||||||||
transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
transactor.Signer = func(address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) { | ||||||||||||||||||||||
locker = t.nonceMux.Lock(chainID) | ||||||||||||||||||||||
|
@@ -421,12 +421,60 @@ | |||||||||||||||||||||
//nolint: wrapcheck | ||||||||||||||||||||||
return parentTransactor.Signer(address, transaction) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
//tmpdebug | ||||||||||||||||||||||
fmt.Printf("test ver 7\n") | ||||||||||||||||||||||
|
||||||||||||||||||||||
// if dynamic gas estimation is not enabled, use cfg var gas_estimate as a gas limit default and do not run a pre-flight simulation | ||||||||||||||||||||||
// since we do not need it to determine proper gas units | ||||||||||||||||||||||
if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) { | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prevent potential integer overflow in chain ID conversion. The conversion of - if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) {
+ chainIDInt := chainID.Int64()
+ if chainIDInt > math.MaxInt32 {
+ return 0, fmt.Errorf("chain ID %d exceeds maximum supported value", chainIDInt)
+ }
+ if !t.config.GetDynamicGasEstimate(int(chainIDInt)) { Also applies to: 431-431, 443-443 🧰 Tools🪛 golangci-lint (1.62.2)[high] 426-426: G115: integer overflow conversion uint64 -> int (gosec) 🪛 GitHub Check: Lint (ethergo)[failure] 426-426: |
||||||||||||||||||||||
|
||||||||||||||||||||||
//tmpdebug | ||||||||||||||||||||||
fmt.Printf("Using Default \n") | ||||||||||||||||||||||
|
||||||||||||||||||||||
transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
|
||||||||||||||||||||||
// //tmpdebug | ||||||||||||||||||||||
// fmt.Printf("SubmitTransaction>forGasEst call \n") | ||||||||||||||||||||||
|
||||||||||||||||||||||
// transactor_forGasEstimate := copyTransactOpts(transactor) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// transactor_forGasEstimate.Nonce.Add(transactor_forGasEstimate.Nonce, big.NewInt(1)) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// tx_forGasEstimate, err := call(transactor_forGasEstimate) | ||||||||||||||||||||||
// if err != nil { | ||||||||||||||||||||||
// return 0, fmt.Errorf("err contract call for gas est: %w", err) | ||||||||||||||||||||||
// } | ||||||||||||||||||||||
|
||||||||||||||||||||||
// fmt.Printf("tx_forGasEstimate: %v\n", tx_forGasEstimate.Gas()) | ||||||||||||||||||||||
|
||||||||||||||||||||||
//transactor.GasLimit = tx_forGasEstimate.Gas() + 555 | ||||||||||||||||||||||
|
||||||||||||||||||||||
// if arbitrum, spoof some low gas units to induce bump tests | ||||||||||||||||||||||
if chainID.Uint64() == 10 { | ||||||||||||||||||||||
transactor.GasLimit = 0 | ||||||||||||||||||||||
} else if chainID.Uint64() == 42161 { | ||||||||||||||||||||||
transactor.GasLimit = 200000 | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
Comment on lines
+454
to
+460
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove hard-coded gas limits for specific chains. Using hard-coded gas limits (0 for chain ID 10 and 200000 for chain ID 42161) is dangerous as it:
Remove the hard-coded values and use proper gas estimation: - // if arbitrum, spoof some low gas units to induce bump tests
- if chainID.Uint64() == 10 {
- transactor.GasLimit = 0
- } else if chainID.Uint64() == 42161 {
- transactor.GasLimit = 200000
- } 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: codecov/patch[warning] 455-455: ethergo/submitter/submitter.go#L455 [warning] 457-457: ethergo/submitter/submitter.go#L457 [warning] 459-459: ethergo/submitter/submitter.go#L459 |
||||||||||||||||||||||
|
||||||||||||||||||||||
//tmpdebug | ||||||||||||||||||||||
fmt.Printf("transactor.GasLimit: %d\n", transactor.GasLimit) | ||||||||||||||||||||||
|
||||||||||||||||||||||
tx, err := call(transactor) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
return 0, fmt.Errorf("could not call contract: %w", err) | ||||||||||||||||||||||
return 0, fmt.Errorf("err contract call for tx: %w", err) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
//tmpdebug | ||||||||||||||||||||||
fmt.Printf("tx.Gas: %d\n", tx.Gas()) | ||||||||||||||||||||||
|
||||||||||||||||||||||
defer locker.Unlock() | ||||||||||||||||||||||
|
||||||||||||||||||||||
//tmpdebug | ||||||||||||||||||||||
fmt.Printf("SubmitTransaction>storeTX\n") | ||||||||||||||||||||||
|
||||||||||||||||||||||
// now that we've stored the tx | ||||||||||||||||||||||
err = t.storeTX(ctx, tx, db.Stored, uuid.New().String()) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
|
@@ -436,6 +484,9 @@ | |||||||||||||||||||||
span.AddEvent("trigger reprocess") | ||||||||||||||||||||||
t.triggerProcessQueue(ctx) | ||||||||||||||||||||||
|
||||||||||||||||||||||
//tmpdebug | ||||||||||||||||||||||
fmt.Printf("SubmitTransaction>tx.Nonce: %d\n", tx.Nonce()) | ||||||||||||||||||||||
|
||||||||||||||||||||||
return tx.Nonce(), nil | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -676,18 +727,31 @@ | |||||||||||||||||||||
|
||||||||||||||||||||||
// getGasEstimate gets the gas estimate for the given transaction. | ||||||||||||||||||||||
// TODO: handle l2s w/ custom gas pricing through contracts. | ||||||||||||||||||||||
func (t *txSubmitterImpl) getGasEstimate(ctx context.Context, chainClient client.EVM, chainID int, tx *types.Transaction) (gasEstimate uint64, err error) { | ||||||||||||||||||||||
func (t *txSubmitterImpl) getGasEstimate(ctx context.Context, chainClient client.EVM, chainID int, tx *types.Transaction) (gasLimit_new uint64, err error) { | ||||||||||||||||||||||
|
||||||||||||||||||||||
Comment on lines
+735
to
+736
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Follow Go naming conventions. The variable name Apply this diff: -func (t *txSubmitterImpl) getGasEstimate(ctx context.Context, chainClient client.EVM, chainID int, tx *types.Transaction) (gasLimit_new uint64, err error) {
+func (t *txSubmitterImpl) getGasEstimate(ctx context.Context, chainClient client.EVM, chainID int, tx *types.Transaction) (gasLimitNew uint64, err error) { 📝 Committable suggestion
Suggested change
🧰 Tools🪛 golangci-lint (1.62.2)[warning] 735-735: var-naming: don't use underscores in Go names; method result gasLimit_new should be gasLimitNew (revive) 🪛 GitHub Check: codecov/patch[warning] 735-737: ethergo/submitter/submitter.go#L735-L737 |
||||||||||||||||||||||
// if dynamic gas estimation is not enabled, use cfg var gas_estimate as a default | ||||||||||||||||||||||
if !t.config.GetDynamicGasEstimate(chainID) { | ||||||||||||||||||||||
return t.config.GetGasEstimate(chainID), nil | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
//tmpdebug | ||||||||||||||||||||||
fmt.Println("getGasEstimate>start") | ||||||||||||||||||||||
|
||||||||||||||||||||||
gasUnitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(chainID) | ||||||||||||||||||||||
|
||||||||||||||||||||||
//tmpdebug | ||||||||||||||||||||||
fmt.Println("getGasEstimate>gasUnitAddPercentage", gasUnitAddPercentage) | ||||||||||||||||||||||
|
||||||||||||||||||||||
ctx, span := t.metrics.Tracer().Start(ctx, "submitter.getGasEstimate", trace.WithAttributes( | ||||||||||||||||||||||
attribute.Int(metrics.ChainID, chainID), | ||||||||||||||||||||||
|
||||||||||||||||||||||
attribute.String(metrics.TxHash, tx.Hash().String()), | ||||||||||||||||||||||
|
||||||||||||||||||||||
attribute.Int("gasUnitAddPercentage", gasUnitAddPercentage), | ||||||||||||||||||||||
)) | ||||||||||||||||||||||
|
||||||||||||||||||||||
defer func() { | ||||||||||||||||||||||
span.AddEvent("estimated_gas", trace.WithAttributes(attribute.Int64("gas", int64(gasEstimate)))) | ||||||||||||||||||||||
span.AddEvent("estimated_gas", trace.WithAttributes(attribute.Int64("gas", int64(gasLimit_new)))) | ||||||||||||||||||||||
metrics.EndSpanWithErr(span, err) | ||||||||||||||||||||||
}() | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -696,21 +760,35 @@ | |||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
return 0, fmt.Errorf("could not convert tx to call: %w", err) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
// tmpdebug | ||||||||||||||||||||||
fmt.Printf("Debug Calling EstimateGas") | ||||||||||||||||||||||
|
||||||||||||||||||||||
gasEstimate, err = chainClient.EstimateGas(ctx, *call) | ||||||||||||||||||||||
// bump gas limit from prior by X% | ||||||||||||||||||||||
gasLimit_fromPrior := tx.Gas() | ||||||||||||||||||||||
|
||||||||||||||||||||||
gasLimit_fromEstimate, err := chainClient.EstimateGas(ctx, *call) | ||||||||||||||||||||||
|
||||||||||||||||||||||
Comment on lines
+770
to
+773
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Follow Go naming conventions for variables. Variable names should follow Go naming conventions. Apply this diff: - gasLimit_fromPrior := tx.Gas()
- gasLimit_fromEstimate, err := chainClient.EstimateGas(ctx, *call)
+ gasLimitFromPrior := tx.Gas()
+ gasLimitFromEstimate, err := chainClient.EstimateGas(ctx, *call) 📝 Committable suggestion
Suggested change
🧰 Tools🪛 golangci-lint (1.62.2)[warning] 770-770: var-naming: don't use underscores in Go names; var gasLimit_fromPrior should be gasLimitFromPrior (revive) [warning] 772-772: var-naming: don't use underscores in Go names; var gasLimit_fromEstimate should be gasLimitFromEstimate (revive) 🪛 GitHub Check: codecov/patch[warning] 770-773: ethergo/submitter/submitter.go#L770-L773 |
||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
span.AddEvent("could not estimate gas", trace.WithAttributes(attribute.String("error", err.Error()))) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// tmpdebug | ||||||||||||||||||||||
fmt.Printf("Debug Default Gas Estimate: %d\n", t.config.GetGasEstimate(chainID)) | ||||||||||||||||||||||
//tmpdebug | ||||||||||||||||||||||
fmt.Printf("getGasEstimate> Error estimating gas: %v\n", err) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// fallback to default | ||||||||||||||||||||||
return t.config.GetGasEstimate(chainID), nil | ||||||||||||||||||||||
span.AddEvent("could not estimate gas", trace.WithAttributes(attribute.String("error", err.Error()))) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// if we failed to est gas for any reason, use *at least* the default flat gas on the next bump | ||||||||||||||||||||||
gasLimit_fromEstimate = max(t.config.GetGasEstimate(chainID), gasLimit_fromEstimate) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
return gasEstimate, nil | ||||||||||||||||||||||
// use whichever is higher, gas from the prior attempt, or gas from our latest simulation estimate | ||||||||||||||||||||||
gasLimit_new = max(gasLimit_fromPrior, gasLimit_fromEstimate) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// whichever source is used as the base, multiply it by the configured gas unit add percentage | ||||||||||||||||||||||
gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100) | ||||||||||||||||||||||
Check failure on line 784 in ethergo/submitter/submitter.go GitHub Actions / Lint (ethergo)
|
||||||||||||||||||||||
Comment on lines
+788
to
+789
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protect against integer overflow in gas limit calculation. The gas limit calculation could overflow when multiplying by the percentage. Apply this diff to use big.Int for safe calculation: - gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100)
+ increase := new(big.Int).SetUint64(gasLimitNew)
+ increase.Mul(increase, big.NewInt(int64(gasUnitAddPercentage)))
+ increase.Div(increase, big.NewInt(100))
+ if !increase.IsUint64() {
+ return 0, fmt.Errorf("gas limit increase exceeds uint64 max value")
+ }
+ gasLimitNew += increase.Uint64()
🧰 Tools🪛 golangci-lint (1.62.2)789-789: assignOp: replace (gocritic) 🪛 GitHub Check: Lint (ethergo)[failure] 789-789: |
||||||||||||||||||||||
span.AddEvent("new gas limit", trace.WithAttributes( | ||||||||||||||||||||||
attribute.Int64("gas_limit", int64(gasLimit_new)), | ||||||||||||||||||||||
attribute.Int64("gas_limit_from_prior", int64(gasLimit_fromPrior)), | ||||||||||||||||||||||
attribute.Int64("gas_limit_from_estimate", int64(gasLimit_fromEstimate)), | ||||||||||||||||||||||
)) | ||||||||||||||||||||||
Comment on lines
+790
to
+794
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protect against integer overflow in trace attributes. The conversion from uint64 to int64 could overflow. Apply this diff: - attribute.Int64("gas_limit", int64(gasLimit_new)),
- attribute.Int64("gas_limit_from_prior", int64(gasLimit_fromPrior)),
- attribute.Int64("gas_limit_from_estimate", int64(gasLimit_fromEstimate)),
+ attribute.String("gas_limit", fmt.Sprintf("%d", gasLimitNew)),
+ attribute.String("gas_limit_from_prior", fmt.Sprintf("%d", gasLimitFromPrior)),
+ attribute.String("gas_limit_from_estimate", fmt.Sprintf("%d", gasLimitFromEstimate)), 📝 Committable suggestion
Suggested change
🧰 Tools🪛 golangci-lint (1.62.2)[high] 791-791: G115: integer overflow conversion uint64 -> int64 (gosec) [high] 792-792: G115: integer overflow conversion uint64 -> int64 (gosec) [high] 793-793: G115: integer overflow conversion uint64 -> int64 (gosec) 🪛 GitHub Check: Lint (ethergo)[failure] 791-791: [failure] 792-792: [failure] 793-793: |
||||||||||||||||||||||
|
||||||||||||||||||||||
return gasLimit_new, nil | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
func (t *txSubmitterImpl) Address() common.Address { | ||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,6 +198,21 @@ func (s *SubmitterSuite) TestGroupTxesByNonce() { | |
} | ||
} | ||
|
||
func (s *SubmitterSuite) TestOpStackGas() { | ||
|
||
mockTx := mocks.GetMockTxes(s.GetTestContext(), s.T(), 1, types.LegacyTxType)[0] | ||
|
||
fmt.Printf("Original Transaction Gas Limit: %d\n", mockTx.Gas()) | ||
|
||
} | ||
|
||
func TestBox(t *testing.T) { | ||
const testTxCount = 10 | ||
mockTx := mocks.GetMockTxes(context.Background(), t, testTxCount, 0) | ||
|
||
fmt.Printf("Original Transaction Gas Limit: %d\n", mockTx[0].Gas()) | ||
} | ||
Comment on lines
+209
to
+214
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance test with assertions. |
||
|
||
// Test for the outersection function. | ||
func TestOutersection(t *testing.T) { | ||
set := []*big.Int{ | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -74,6 +74,9 @@ | |||||||||
gasAmount := big.NewInt(0) | ||||||||||
var err error | ||||||||||
|
||||||||||
//tmpdebug | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix comment formatting to follow Go standards. The comments don't follow Go formatting standards. There should be a space between Apply this diff to fix the formatting: - //tmpdebug
+ // tmpdebug Also applies to: 92-92, 98-98 🧰 Tools🪛 golangci-lint (1.62.2)77-77: commentFormatting: put a space between (gocritic) 🪛 GitHub Check: Lint (services/rfq)[failure] 77-77: |
||||||||||
fmt.Println("SubmitRelay>start: ", request.OriginTxHash) | ||||||||||
Comment on lines
+77
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Replace debug print with structured logging. Replace temporary debug print with proper logging using the existing Apply this diff to improve the code: - //tmpdebug
- fmt.Println("SubmitRelay>start: ", request.OriginTxHash)
+ logger.Debugw("starting relay submission",
+ "origin_tx_hash", request.OriginTxHash) 📝 Committable suggestion
Suggested change
🧰 Tools🪛 golangci-lint (1.62.2)77-77: commentFormatting: put a space between (gocritic) 🪛 GitHub Check: Lint (services/rfq)[failure] 77-77: |
||||||||||
|
||||||||||
// Check to see if ETH should be sent to destination | ||||||||||
if util.IsGasToken(request.Transaction.DestToken) { | ||||||||||
gasAmount = request.Transaction.DestAmount | ||||||||||
|
@@ -86,19 +89,37 @@ | |||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
//tmpdebug | ||||||||||
fmt.Println("SubmitRelay>SubmitTransaction: ", request.OriginTxHash) | ||||||||||
|
||||||||||
nonce, err := c.SubmitTransaction(ctx, func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||||||||||
transactor.Value = core.CopyBigInt(gasAmount) | ||||||||||
|
||||||||||
//tmpdebug | ||||||||||
callType := "exec" | ||||||||||
if transactor.GasLimit == 0 { | ||||||||||
callType = "sim" | ||||||||||
} | ||||||||||
|
||||||||||
//tmpdebug | ||||||||||
fmt.Println(callType, "SubmitTransaction>RelayV2: ", request.OriginTxHash) | ||||||||||
|
||||||||||
tx, err = c.Bridge.RelayV2(transactor, request.RawRequest, c.submitter.Address()) | ||||||||||
|
||||||||||
if err != nil { | ||||||||||
return nil, fmt.Errorf("could not relay: %w", err) | ||||||||||
} | ||||||||||
|
||||||||||
//tmpdebug | ||||||||||
fmt.Println(callType, "RelayV2 Return tx hash:", request.OriginTxHash, tx.Hash()) | ||||||||||
|
||||||||||
return tx, nil | ||||||||||
}) | ||||||||||
if err != nil { | ||||||||||
return 0, nil, fmt.Errorf("could not submit transaction: %w", err) | ||||||||||
} | ||||||||||
//tmpdebug | ||||||||||
fmt.Println("SubmitRelay nonce:", nonce, "gas amount:", gasAmount) | ||||||||||
|
||||||||||
return nonce, gasAmount, nil | ||||||||||
} |
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.
🛠️ Refactor suggestion
Enhance test coverage for the new configuration parameter.
While the basic YAML parsing test is good, consider adding the following test cases:
TestGetters
forGetDynamicGasUnitAddPercentage
Here's a suggested implementation: