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

+dynamic_gas_unit_add_percentage flag #3494

Merged
merged 42 commits into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from 41 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
dfbf6ba
+dynamic_gas_unit_add_percentage flag
parodime Jan 23, 2025
93459f1
test fix
parodime Jan 24, 2025
905a3a5
[goreleaser]
parodime Jan 24, 2025
09816fb
new simulate approach
parodime Jan 24, 2025
63f730c
[goreleaser]
parodime Jan 24, 2025
31ca3cf
fix tx.Hash empty [goreleaser]
parodime Jan 24, 2025
d8481f2
remove debug noise. nil deref fix... ?
parodime Jan 24, 2025
d7f1cad
[goreleaser]
parodime Jan 24, 2025
3d7eea2
check tx call b4 estgas [goreleaser]
parodime Jan 24, 2025
4c11d33
+tmp debuggers [goreleaser]
parodime Jan 27, 2025
145583d
more tmp debug [goreleaser]
parodime Jan 27, 2025
2005d91
tmp debug tx output [goreleaser]
parodime Jan 27, 2025
a8a48bb
tmp debug submit tweaks [goreleaser]
parodime Jan 27, 2025
75c9c44
submitTransaction refactor [goreleaser]
parodime Jan 27, 2025
96e16c3
clone transactor [goreleaser]
parodime Jan 27, 2025
dada2b0
deepcopy [goreleaser]
parodime Jan 27, 2025
7abfdb0
[goreleaser]
parodime Jan 27, 2025
0e7c236
test getGasEstimate skip
parodime Jan 27, 2025
220e0d4
[goreleaser]
parodime Jan 27, 2025
b995385
try gaslimit 0 [goreleaser]
parodime Jan 28, 2025
4782b44
gaslimit2 [goreleaser]
parodime Jan 28, 2025
d699cd0
gaslimit2 pre/post [goreleaser]
parodime Jan 28, 2025
04ccdc2
tx_forGasEst [goreleaser]
parodime Jan 28, 2025
c8f2dfe
diff nonces [goreleaser]
parodime Jan 28, 2025
d199850
Prove gas [goreleaser]
parodime Jan 28, 2025
e8fd10e
print nonce [goreleaser]
parodime Jan 28, 2025
ee9eb85
low gas test [goreleaser]
parodime Jan 28, 2025
f6bfa43
getGasEst bump approach [goreleaser]
parodime Jan 28, 2025
c79a81b
reproduce [goreleaser]
parodime Jan 28, 2025
82b4f99
repro err [goreleaser]
parodime Jan 28, 2025
5b02cca
bump approach [goreleaser]
parodime Jan 28, 2025
965802d
bump approach - arbi test focus [goreleaser]
parodime Jan 29, 2025
7b62df2
swap context [goreleaser]
parodime Jan 29, 2025
73442fa
fix transactor_forGasEstimate lock. +QuoteDetails event
parodime Jan 31, 2025
ab18ea9
quotedetails event fix. +basic print logs
parodime Jan 31, 2025
cf43fdc
print log impvs [goreleaser]
parodime Jan 31, 2025
c24c568
typo fix
parodime Jan 31, 2025
0e35f40
gitignore edit
parodime Jan 31, 2025
8adb278
getGasEstimate code simplify
parodime Jan 31, 2025
961fbfc
remove temp tests
parodime Jan 31, 2025
6586d73
remove underscores, FormatError util func,
parodime Jan 31, 2025
528839f
submitTransaction use common performSignature func for sim & actual
parodime Jan 31, 2025
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
7 changes: 7 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,10 @@ main

# golang-ci-lint binary
contrib/golang-ci-lint/golang-ci-lint

*signer*.txt

# ignore any rfq config files that are not the template
services/rfq/**/config*.yaml
services/rfq/**/config*.yml
!services/rfq/**/config.yml
3 changes: 3 additions & 0 deletions docs/bridge/docs/06-Services/04-Submitter.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ submitter_config:
dynamic_gas_estimate: true
# SupportsEIP1559 is whether or not this chain supports EIP1559.
supports_eip_1559: true
# DynamicGasUnitAddPercentage - increase gas unit limit (ie: "gas" field on a typical tx) by X% from what dynamic gas estimate returns
# Has no effect if dynamic gas estimation is not also enabled.
dynamic_gas_unit_add_percentage: 5
43114:
max_gas_price: 100000000000 # 100 Gwei
10:
Expand Down
28 changes: 26 additions & 2 deletions ethergo/submitter/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@
DynamicGasEstimate bool `yaml:"dynamic_gas_estimate"`
// SupportsEIP1559 is whether or not this chain supports EIP1559
SupportsEIP1559 bool `yaml:"supports_eip_1559"`
// DynamicGasUnitAddPercentage - increase gas unit limit (ie: "gas" field on a typical tx) by X% from what dynamic gas estimate returns
// Has no effect if dynamic gas estimation is not also enabled.
DynamicGasUnitAddPercentage int `yaml:"dynamic_gas_unit_add_percentage"`
}

const (
Expand All @@ -64,6 +67,9 @@

// DefaultGasEstimate is the default gas estimate to use for transactions.
DefaultGasEstimate = uint64(1200000)

// DefaultDynamicGasUnitAddPercentage is the default percentage to bump the gas limit by.
DefaultDynamicGasUnitAddPercentage = 5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure we don't want to default to zero? This is fine with me, just making sure since may not be obvious to others

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with zaps we're going to need to start doing dynamic estimates all the time and relying on the simulation to give us enough limit. a small boost by default on top of the simulation is a good idea just to further reduce chance of OOG

also -- our flat limits that we use today, by comparison, give us a boost of like +5000% so +5% is pretty small by that measure

Copy link
Collaborator

Choose a reason for hiding this comment

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

For reference, most wallets are using a 50% buffer on top of the estimated gas limit. So I'd recommend increasing the default value a bit here.

)

// DefaultMaxPrice is the default max price of a tx.
Expand Down Expand Up @@ -188,19 +194,37 @@
return gasBumpPercentage
}

// GetDynamicGasUnitAddPercentage returns the percentage to bump the gas limit by

Check failure on line 197 in ethergo/submitter/config/config.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

Comment should end in a period (godot)
func (c *Config) GetDynamicGasUnitAddPercentage(chainID int) (dynamicGasUnitAddPercentage int) {
chainConfig, ok := c.Chains[chainID]
if ok {
dynamicGasUnitAddPercentage = chainConfig.DynamicGasUnitAddPercentage
}

Check warning on line 202 in ethergo/submitter/config/config.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/config/config.go#L198-L202

Added lines #L198 - L202 were not covered by tests
// if dynamicGasUnitAddPercentage is not set for the chain, use the global config
if dynamicGasUnitAddPercentage == 0 {
dynamicGasUnitAddPercentage = c.DynamicGasUnitAddPercentage
}

Check warning on line 206 in ethergo/submitter/config/config.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/config/config.go#L204-L206

Added lines #L204 - L206 were not covered by tests

// if the dynamicGasUnitAddPercentage isn't set at all, use the default
if dynamicGasUnitAddPercentage == 0 {
dynamicGasUnitAddPercentage = DefaultDynamicGasUnitAddPercentage
}
return dynamicGasUnitAddPercentage

Check warning on line 212 in ethergo/submitter/config/config.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/config/config.go#L209-L212

Added lines #L209 - L212 were not covered by tests
}

// GetGasEstimate returns the gas estimate to use for transactions
// TODO: test this method.
func (c *Config) GetGasEstimate(chainID int) (gasEstimate uint64) {
chainConfig, ok := c.Chains[chainID]
if ok {
gasEstimate = chainConfig.GasEstimate
}
// if gasBumpPercentage is not set for the chain, use the global config
// if gasEstimate is not set for the chain, use the global config
if gasEstimate == 0 {
gasEstimate = c.GasEstimate
}

// if the gasBumpPercentage isn't set at all, use the default
// if the gasEstimate isn't set at all, use the default
if gasEstimate == 0 {
gasEstimate = DefaultGasEstimate
}
Expand Down
2 changes: 2 additions & 0 deletions ethergo/submitter/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,15 @@ gas_bump_percentage: 10
gas_estimate: 1000
is_l2: true
dynamic_gas_estimate: true
dynamic_gas_unit_add_percentage: 20
supports_eip_1559: true`
var cfg config.Config
err := yaml.Unmarshal([]byte(cfgStr), &cfg)
assert.NoError(t, err)
assert.Equal(t, big.NewInt(250000000000), cfg.MaxGasPrice)
assert.Equal(t, 60, cfg.BumpIntervalSeconds)
assert.Equal(t, 10, cfg.GasBumpPercentage)
assert.Equal(t, 20, cfg.DynamicGasUnitAddPercentage)
Copy link
Contributor

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:

  1. Add a test case in TestGetters for GetDynamicGasUnitAddPercentage
  2. Test the default value (5%)
  3. Add validation tests for edge cases (negative values, zero, etc.)

Here's a suggested implementation:

 func TestGetters(t *testing.T) {
     cfg := config.Config{
         ChainConfig: config.ChainConfig{
             MaxBatchSize: 5,
             DoNotBatch:   false,
             MaxGasPrice:  big.NewInt(250 * params.GWei),
+            DynamicGasUnitAddPercentage: 15,
         },
         Chains: map[int]config.ChainConfig{
                 MaxBatchSize: 8,
                 DoNotBatch:   true,
                 MaxGasPrice:  big.NewInt(300 * params.GWei),
+                DynamicGasUnitAddPercentage: 25,
             },
                 MaxBatchSize: 0, // Should use global config value
             },
         },
     }

     // ... existing test cases ...

+    t.Run("GetDynamicGasUnitAddPercentage", func(t *testing.T) {
+        // Test chain-specific value
+        assert.Equal(t, 25, cfg.GetDynamicGasUnitAddPercentage(1))
+        // Test fallback to global value
+        assert.Equal(t, 15, cfg.GetDynamicGasUnitAddPercentage(2))
+        // Test nonexistent chain
+        assert.Equal(t, 15, cfg.GetDynamicGasUnitAddPercentage(999))
+
+        // Test default value
+        emptyCfg := config.Config{}
+        assert.Equal(t, config.DefaultDynamicGasUnitAddPercentage, 
+            emptyCfg.GetDynamicGasUnitAddPercentage(1))
+
+        // Test validation (if implemented)
+        invalidCfg := config.Config{
+            ChainConfig: config.ChainConfig{
+                DynamicGasUnitAddPercentage: -1,
+            },
+        }
+        assert.Equal(t, config.DefaultDynamicGasUnitAddPercentage,
+            invalidCfg.GetDynamicGasUnitAddPercentage(1))
+    })
 }

Committable suggestion skipped: line range outside the PR's diff.

assert.Equal(t, uint64(1000), cfg.GasEstimate)
assert.Equal(t, true, cfg.DynamicGasEstimate)
assert.Equal(t, true, cfg.SupportsEIP1559(0))
Expand Down
2 changes: 2 additions & 0 deletions ethergo/submitter/config/iconfig_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

74 changes: 60 additions & 14 deletions ethergo/submitter/submitter.go
Original file line number Diff line number Diff line change
Expand Up @@ -392,9 +392,6 @@
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)
Expand All @@ -421,10 +418,54 @@
//nolint: wrapcheck
return parentTransactor.Signer(address, transaction)
}

// 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())) {

Check failure on line 424 in ethergo/submitter/submitter.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

G115: integer overflow conversion uint64 -> int (gosec)
parodime marked this conversation as resolved.
Show resolved Hide resolved
transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64()))

Check failure on line 425 in ethergo/submitter/submitter.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

G115: integer overflow conversion uint64 -> int (gosec)
parodime marked this conversation as resolved.
Show resolved Hide resolved
} else {

Check failure on line 426 in ethergo/submitter/submitter.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

unnecessary leading newline (whitespace)
Comment on lines +429 to +431
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Protect against integer overflow in chain ID conversion.

The conversion of chainID.Uint64() to int could overflow on 32-bit systems.

-	if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) {
-		transactor.GasLimit = t.config.GetGasEstimate(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)) {
+		transactor.GasLimit = t.config.GetGasEstimate(int(chainIDInt))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) {
transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64()))
} else {
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)) {
transactor.GasLimit = t.config.GetGasEstimate(int(chainIDInt))
} else {
🧰 Tools
🪛 golangci-lint (1.62.2)

[high] 424-424: G115: integer overflow conversion uint64 -> int

(gosec)


[high] 425-425: G115: integer overflow conversion uint64 -> int

(gosec)


426-426: unnecessary leading newline

(whitespace)

🪛 GitHub Check: Lint (ethergo)

[failure] 424-424:
G115: integer overflow conversion uint64 -> int (gosec)


[failure] 425-425:
G115: integer overflow conversion uint64 -> int (gosec)


[failure] 426-426:
unnecessary leading newline (whitespace)


// deepcopy the real transactor so we can use it for simulation

Check warning on line 428 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L427-L428

Added lines #L427 - L428 were not covered by tests
transactorForGasEstimate := copyTransactOpts(transactor)

// override the signer func for our simulation/estimation with a version that does not lock the nonce,
// which would othewrise cause a deadlock with the following *actual* transactor

Check warning on line 432 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L430-L432

Added lines #L430 - L432 were not covered by tests
transactorForGasEstimate.Signer = func(address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) {

Check failure on line 433 in ethergo/submitter/submitter.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

unnecessary leading newline (whitespace)

newNonce, err := t.getNonce(ctx, chainID, address)
if err != nil {
return nil, fmt.Errorf("could not sign tx: %w", err)
}

Check warning on line 438 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L434-L438

Added lines #L434 - L438 were not covered by tests

txType := t.txTypeForChain(chainID)

transaction, err = util.CopyTX(transaction, util.WithNonce(newNonce), util.WithTxType(txType))
if err != nil {
return nil, fmt.Errorf("could not copy tx: %w", err)
}

Check warning on line 445 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L440-L445

Added lines #L440 - L445 were not covered by tests

//nolint: wrapcheck
return parentTransactor.Signer(address, transaction)

Check warning on line 448 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L448

Added line #L448 was not covered by tests
}

txForGasEstimate, err := call(transactorForGasEstimate)
if err != nil {

Check warning on line 452 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L452

Added line #L452 was not covered by tests
errMsg := util.FormatError(err)

return 0, fmt.Errorf("err contract call for gas est: %s", errMsg)

Check warning on line 455 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L455

Added line #L455 was not covered by tests
}

// with our gas limit now obtained from the simulation, apply this limit (plus any configured % modifier) to the
// gas limit of the actual transactor that is about to prepare the real transaction
gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainID.Uint64()))

Check warning on line 460 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L460

Added line #L460 was not covered by tests

Check failure on line 460 in ethergo/submitter/submitter.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

G115: integer overflow conversion uint64 -> int (gosec)
transactor.GasLimit = txForGasEstimate.Gas() + (txForGasEstimate.Gas() * uint64(gasLimitAddPercentage) / 100)

Check failure on line 461 in ethergo/submitter/submitter.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

G115: integer overflow conversion int -> uint64 (gosec)
Comment on lines +451 to +452
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Protect against integer overflow in gas limit calculation.

The gas limit percentage calculation could overflow when dealing with large gas values.

-		gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainID.Uint64()))
-		transactor.GasLimit = txForGasEstimate.Gas() + (txForGasEstimate.Gas() * uint64(gasLimitAddPercentage) / 100)
+		chainIDInt := chainID.Int64()
+		if chainIDInt > math.MaxInt32 {
+			return 0, fmt.Errorf("chain ID %d exceeds maximum supported value", chainIDInt)
+		}
+		gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainIDInt))
+		
+		// Use big.Int for safe calculation
+		gasLimit := new(big.Int).SetUint64(txForGasEstimate.Gas())
+		increase := new(big.Int).Mul(gasLimit, big.NewInt(int64(gasLimitAddPercentage)))
+		increase.Div(increase, big.NewInt(100))
+		if !increase.IsUint64() {
+			return 0, fmt.Errorf("gas limit increase exceeds uint64 max value")
+		}
+		transactor.GasLimit = txForGasEstimate.Gas() + increase.Uint64()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainID.Uint64()))
transactor.GasLimit = txForGasEstimate.Gas() + (txForGasEstimate.Gas() * uint64(gasLimitAddPercentage) / 100)
chainIDInt := chainID.Int64()
if chainIDInt > math.MaxInt32 {
return 0, fmt.Errorf("chain ID %d exceeds maximum supported value", chainIDInt)
}
gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainIDInt))
// Use big.Int for safe calculation
gasLimit := new(big.Int).SetUint64(txForGasEstimate.Gas())
increase := new(big.Int).Mul(gasLimit, big.NewInt(int64(gasLimitAddPercentage)))
increase.Div(increase, big.NewInt(100))
if !increase.IsUint64() {
return 0, fmt.Errorf("gas limit increase exceeds uint64 max value")
}
transactor.GasLimit = txForGasEstimate.Gas() + increase.Uint64()
🧰 Tools
🪛 golangci-lint (1.62.2)

[high] 460-460: G115: integer overflow conversion uint64 -> int

(gosec)


[high] 461-461: G115: integer overflow conversion int -> uint64

(gosec)

🪛 GitHub Check: codecov/patch

[warning] 460-460: ethergo/submitter/submitter.go#L460
Added line #L460 was not covered by tests

🪛 GitHub Check: Lint (ethergo)

[failure] 460-460:
G115: integer overflow conversion uint64 -> int (gosec)


[failure] 461-461:
G115: integer overflow conversion int -> uint64 (gosec)

}

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)

Check warning on line 466 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L466

Added line #L466 was not covered by tests
}

defer locker.Unlock()

// now that we've stored the tx
Expand Down Expand Up @@ -676,18 +717,23 @@

// 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 uint64, err error) {

// if dynamic gas estimation is not enabled, use cfg var gas_estimate as a default

Check warning on line 722 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L720-L722

Added lines #L720 - L722 were not covered by tests
if !t.config.GetDynamicGasEstimate(chainID) {
return t.config.GetGasEstimate(chainID), nil
}

gasUnitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(chainID)

Check warning on line 728 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L727-L728

Added lines #L727 - L728 were not covered by tests
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),

Check warning on line 732 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L732

Added line #L732 was not covered by tests
parodime marked this conversation as resolved.
Show resolved Hide resolved
))

defer func() {
span.AddEvent("estimated_gas", trace.WithAttributes(attribute.Int64("gas", int64(gasEstimate))))
span.AddEvent("estimated_gas", trace.WithAttributes(attribute.Int64("gas", int64(gasLimit))))

Check warning on line 736 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L736

Added line #L736 was not covered by tests

Check failure on line 736 in ethergo/submitter/submitter.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

G115: integer overflow conversion uint64 -> int64 (gosec)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use string attributes for large gas values.

Converting gas values to int64 in trace attributes could overflow.

Apply this diff:

-       span.AddEvent("estimated_gas", trace.WithAttributes(attribute.Int64("gas", int64(gasLimit))))
+       span.AddEvent("estimated_gas", trace.WithAttributes(attribute.String("gas", fmt.Sprintf("%d", gasLimit))))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
span.AddEvent("estimated_gas", trace.WithAttributes(attribute.Int64("gas", int64(gasLimit))))
span.AddEvent("estimated_gas", trace.WithAttributes(attribute.String("gas", fmt.Sprintf("%d", gasLimit))))
🧰 Tools
🪛 golangci-lint (1.62.2)

[high] 727-727: G115: integer overflow conversion uint64 -> int64

(gosec)

🪛 GitHub Check: codecov/patch

[warning] 727-727: ethergo/submitter/submitter.go#L727
Added line #L727 was not covered by tests

🪛 GitHub Check: Lint (ethergo)

[failure] 727-727:
G115: integer overflow conversion uint64 -> int64 (gosec)

metrics.EndSpanWithErr(span, err)
}()

Expand All @@ -696,21 +742,21 @@
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)
gasLimitFromEstimate, err := chainClient.EstimateGas(ctx, *call)

Check warning on line 747 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L747

Added line #L747 was not covered by tests
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))

// fallback to default
// if we failed to est gas for any reason, use the default flat gas from config

Check warning on line 751 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L751

Added line #L751 was not covered by tests
return t.config.GetGasEstimate(chainID), nil
}

return gasEstimate, nil
// multiply the freshly simulated gasLimit by the configured gas unit add percentage
gasLimitFromEstimate += (gasLimitFromEstimate * uint64(gasUnitAddPercentage) / 100)

Check failure on line 756 in ethergo/submitter/submitter.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

G115: integer overflow conversion int -> uint64 (gosec)
gasLimit = gasLimitFromEstimate
Comment on lines +746 to +748
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Protect against integer overflow in gas limit adjustment.

The gas limit percentage calculation needs overflow protection.

-	gasLimitFromEstimate += (gasLimitFromEstimate * uint64(gasUnitAddPercentage) / 100)
+	// Use big.Int for safe calculation
+	gasLimitBig := new(big.Int).SetUint64(gasLimitFromEstimate)
+	increase := new(big.Int).Mul(gasLimitBig, big.NewInt(int64(gasUnitAddPercentage)))
+	increase.Div(increase, big.NewInt(100))
+	if !increase.IsUint64() {
+		return 0, fmt.Errorf("gas limit increase exceeds uint64 max value")
+	}
+	gasLimitFromEstimate = gasLimitFromEstimate + increase.Uint64()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// multiply the freshly simulated gasLimit by the configured gas unit add percentage
gasLimitFromEstimate += (gasLimitFromEstimate * uint64(gasUnitAddPercentage) / 100)
gasLimit = gasLimitFromEstimate
// multiply the freshly simulated gasLimit by the configured gas unit add percentage
// Use big.Int for safe calculation
gasLimitBig := new(big.Int).SetUint64(gasLimitFromEstimate)
increase := new(big.Int).Mul(gasLimitBig, big.NewInt(int64(gasUnitAddPercentage)))
increase.Div(increase, big.NewInt(100))
if !increase.IsUint64() {
return 0, fmt.Errorf("gas limit increase exceeds uint64 max value")
}
gasLimitFromEstimate = gasLimitFromEstimate + increase.Uint64()
gasLimit = gasLimitFromEstimate
🧰 Tools
🪛 golangci-lint (1.62.2)

[high] 756-756: G115: integer overflow conversion int -> uint64

(gosec)

🪛 GitHub Check: Lint (ethergo)

[failure] 756-756:
G115: integer overflow conversion int -> uint64 (gosec)


return gasLimit, nil

Check warning on line 759 in ethergo/submitter/submitter.go

View check run for this annotation

Codecov / codecov/patch

ethergo/submitter/submitter.go#L758-L759

Added lines #L758 - L759 were not covered by tests
}

func (t *txSubmitterImpl) Address() common.Address {
Expand Down
17 changes: 17 additions & 0 deletions ethergo/util/util.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package util

import "strings"

// FormatError applies custom formatting & noise reduction to error messages. Add more as needed.
func FormatError(err error) string {
if err == nil {
return ""
}
errMsg := err.Error()

//if an error message contains embedded HTML (eg: many RPC errors), strip it out to reduce noise.

Check failure on line 12 in ethergo/util/util.go

View workflow job for this annotation

GitHub Actions / Lint (ethergo)

commentFormatting: put a space between `//` and comment text (gocritic)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix comment formatting.

Add a space after // to comply with Go comment formatting standards.

-	//if an error message contains embedded HTML (eg: many RPC errors), strip it out to reduce noise.
+	// if an error message contains embedded HTML (eg: many RPC errors), strip it out to reduce noise.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
//if an error message contains embedded HTML (eg: many RPC errors), strip it out to reduce noise.
// if an error message contains embedded HTML (eg: many RPC errors), strip it out to reduce noise.
🧰 Tools
🪛 golangci-lint (1.62.2)

12-12: commentFormatting: put a space between // and comment text

(gocritic)

🪛 GitHub Check: Lint (ethergo)

[failure] 12-12:
commentFormatting: put a space between // and comment text (gocritic)

if strings.Contains(errMsg, "<!DOCTYPE html>") {
errMsg = strings.Split(errMsg, "<!DOCTYPE html>")[0] + "<html portion of error removed>"
}
return errMsg
}
14 changes: 14 additions & 0 deletions services/rfq/contracts/fastbridgev2/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ var (
BridgeDepositClaimedTopic common.Hash
// BridgeProofDisputedTopic is the topic emitted by a bridge dispute.
BridgeProofDisputedTopic common.Hash
// BridgeQuoteDetailsTopic is a secondary topic emitted by a bridge request.
BridgeQuoteDetailsTopic common.Hash
)

// static checks to make sure topics actually exist.
Expand All @@ -36,6 +38,7 @@ func init() {
BridgeProofProvidedTopic = parsedABI.Events["BridgeProofProvided"].ID
BridgeDepositClaimedTopic = parsedABI.Events["BridgeDepositClaimed"].ID
BridgeProofDisputedTopic = parsedABI.Events["BridgeProofDisputed"].ID
BridgeQuoteDetailsTopic = parsedABI.Events["BridgeQuoteDetails"].ID

_, err = parsedABI.EventByID(BridgeRequestedTopic)
if err != nil {
Expand All @@ -56,6 +59,16 @@ func init() {
if err != nil {
panic(err)
}

_, err = parsedABI.EventByID(BridgeDepositClaimedTopic)
if err != nil {
panic(err)
}

_, err = parsedABI.EventByID(BridgeQuoteDetailsTopic)
if err != nil {
panic(err)
}
}

// topicMap maps events to topics.
Expand All @@ -67,6 +80,7 @@ func topicMap() map[EventType]common.Hash {
BridgeProofProvidedEvent: BridgeProofProvidedTopic,
BridgeDepositClaimedEvent: BridgeDepositClaimedTopic,
BridgeDisputeEvent: BridgeProofDisputedTopic,
BridgeQuoteDetailsEvent: BridgeQuoteDetailsTopic,
}
}

Expand Down
5 changes: 3 additions & 2 deletions services/rfq/contracts/fastbridgev2/eventtype_string.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions services/rfq/contracts/fastbridgev2/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
BridgeDepositClaimedEvent
// BridgeDisputeEvent is the event type for the BridgeDispute event.
BridgeDisputeEvent
// BridgeQuoteDetailsEvent is emitted along w/ BridgeRequestedEvent as supplemental data

Check failure on line 26 in services/rfq/contracts/fastbridgev2/parser.go

View workflow job for this annotation

GitHub Actions / Lint (services/rfq)

Comment should end in a period (godot)
BridgeQuoteDetailsEvent
)

// Parser parses events from the fastbridge contracat.
Expand Down Expand Up @@ -91,6 +93,12 @@
return noOpEvent, nil, false
}
return eventType, disputed, true
case BridgeQuoteDetailsEvent:
quoteDetails, err := p.filterer.ParseBridgeQuoteDetails(log)
if err != nil {
return noOpEvent, nil, false
}
return eventType, quoteDetails, true
}

return eventType, nil, true
Expand Down
9 changes: 9 additions & 0 deletions services/rfq/relayer/chain/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,19 @@ func (c Chain) SubmitRelay(ctx context.Context, request reldb.QuoteRequest) (uin
}
}

fmt.Printf(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we trace this instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i just added these to help provide some high-level context at a glance in the times that we're observing logs. Not likely to add many more statements like these & will continue to primarily trace everything

"TxID 0x%x %7d.%s > %7d.%s : Submitting \033[32mRelay\033[0m\n",
request.TransactionID,
request.Transaction.OriginChainId,
request.Transaction.OriginToken.Hex()[:6],
request.Transaction.DestChainId,
request.Transaction.DestToken.Hex()[:6])

nonce, err := c.SubmitTransaction(ctx, func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) {
transactor.Value = core.CopyBigInt(gasAmount)

tx, err = c.Bridge.RelayV2(transactor, request.RawRequest, c.submitter.Address())

if err != nil {
return nil, fmt.Errorf("could not relay: %w", err)
}
Expand Down
11 changes: 8 additions & 3 deletions services/rfq/relayer/pricer/fee_pricer.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@
"github.com/jellydator/ttlcache/v3"
"github.com/synapsecns/sanguine/core/metrics"
"github.com/synapsecns/sanguine/ethergo/submitter"
ethergoUtil "github.com/synapsecns/sanguine/ethergo/util"
"github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgev2"
"github.com/synapsecns/sanguine/services/rfq/relayer/chain"
"github.com/synapsecns/sanguine/services/rfq/relayer/relconfig"
"github.com/synapsecns/sanguine/services/rfq/relayer/reldb"
"github.com/synapsecns/sanguine/services/rfq/util"
rfqUtil "github.com/synapsecns/sanguine/services/rfq/util"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/trace"
)
Expand Down Expand Up @@ -255,15 +256,19 @@
Data: encodedData,
}
// Tx.value needs to match `DestAmount` for native gas token, or `ZapNative` for ERC20s.
if util.IsGasToken(quoteRequest.Transaction.DestToken) {
if rfqUtil.IsGasToken(quoteRequest.Transaction.DestToken) {
callMsg.Value = quoteRequest.Transaction.DestAmount
} else {
callMsg.Value = quoteRequest.Transaction.ZapNative
}

// note: this gas amount is intentionally not modified
gasEstimate, err = client.EstimateGas(ctx, callMsg)
if err != nil {

Check failure on line 267 in services/rfq/relayer/pricer/fee_pricer.go

View workflow job for this annotation

GitHub Actions / Lint (services/rfq)

unnecessary leading newline (whitespace)
return 0, fmt.Errorf("could not estimate gas: %w", err)

errMsg := ethergoUtil.FormatError(err)

return 0, fmt.Errorf("could not estimate gas: %s", errMsg)
}

return gasEstimate, nil
Expand Down
Loading
Loading