+dynamic_gas_unit_add_percentage flag #3494
Conversation
WalkthroughThis pull request introduces a new configuration option for dynamic gas unit estimation in the Ethergo Submitter module. The changes primarily focus on adding a Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant Submitter as Transaction Submitter
participant EVMClient as EVM Client
Config->>Submitter: Provide dynamic gas unit percentage
Submitter->>EVMClient: Estimate base gas
Submitter->>Submitter: Calculate adjusted gas limit
Submitter->>EVMClient: Submit transaction with adjusted gas
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/relayer-arb-call #3494 +/- ##
===============================================================
- Coverage 21.97606% 21.96336% -0.01270%
===============================================================
Files 502 503 +1
Lines 41932 41979 +47
===============================================================
+ Hits 9215 9220 +5
- Misses 31779 31821 +42
Partials 938 938
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Bundle ReportChanges will increase total bundle size by 19.37MB (204.78%) ⬆️
Affected Assets, Files, and Routes:view changes for bundle: synapse-interface-client-array-pushAssets Changed:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
ethergo/submitter/config/iconfig_generated.go (1)
44-45: Ensure this method is covered in tests.
The new methodGetDynamicGasUnitAddPercentagelooks appropriate for retrieving the configured percentage. Confirm that test cases adequately cover both a default value scenario and a chain-specific override scenario.ethergo/submitter/util_test.go (1)
201-208: Add assertions or remove unneeded whitespace.
TestOpStackGas only prints the gas limit for a mock transaction and does not perform assertions. Consider adding actual validations for the transaction’s gas to ensure meaningful test coverage. Also, note that the static analysis flagged unnecessary newlines at lines 201 and 207.-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 (s *SubmitterSuite) TestOpStackGas() { + mockTx := mocks.GetMockTxes(s.GetTestContext(), s.T(), 1, types.LegacyTxType)[0] + fmt.Printf("Original Transaction Gas Limit: %d\n", mockTx.Gas()) + // e.g., assert that the gas limit is above 0 or matches an expected range +}🧰 Tools
🪛 golangci-lint (1.62.2)
201-201: unnecessary leading newline
(whitespace)
207-207: unnecessary trailing newline
(whitespace)
🪛 GitHub Check: Lint (ethergo)
[failure] 201-201:
unnecessary leading newline (whitespace)
[failure] 207-207:
unnecessary trailing newline (whitespace)ethergo/submitter/config/config.go (5)
52-54: Add clarity on potential edge cases.
This new fieldDynamicGasUnitAddPercentageis effective only if dynamic gas estimation is enabled. Document any interactions with other fields, such asGasEstimate, to avoid confusion for maintainers.
71-72: Confirm default aligns with usage patterns.
DefaultDynamicGasUnitAddPercentageis set to 5, which might be too high or too low depending on real-world chain usage. Ensure the default works well with typical chain conditions.
197-213: Add a period to end the doc comment.
Per lint guidelines, the comment on line 197 should end with a period. Also consider describing how this method interacts withGetDynamicGasEstimatefor better clarity.-// GetDynamicGasUnitAddPercentage returns the percentage to bump the gas limit by +// GetDynamicGasUnitAddPercentage returns the percentage to bump the gas limit by.🧰 Tools
🪛 GitHub Check: Lint (ethergo)
[failure] 197-197:
Comment should end in a period (godot)
222-222: Inform test coverage or finalize TODO.
This comment clarifies that gasEstimate flows from chain to global. Confirm if any additional test coverage is needed or if you have a related TODO to finalize.
227-227: Keep comment and logic in sync.
The comment references the default fallback forgasEstimate. If updating defaults or logic in the future, remember to keep these explanatory comments aligned to prevent stale documentation.ethergo/submitter/submitter.go (2)
681-692: Validate dynamic gas unit percentage range.You are retrieving
dynamic_gas_unit_add_percentageviaGetDynamicGasUnitAddPercentagebut there's no apparent validation for extremely large or negative values. Consider enforcing a reasonable range to avoid unexpected behavior or overflow when applying the percentage.If needed, add a guard check in the config loading or here:
if gasUnitAddPercentage < 0 || gasUnitAddPercentage > someMax { // handle invalid config }
720-721: Use the shorthand assignment operator and guard against overflow.Replace the repeated addition with the shorthand operator to follow idiomatic Go conventions. Also consider monitoring for potential overflow if
gasEstimateorgasUnitAddPercentageis unexpectedly large.- gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) + gasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)🧰 Tools
🪛 golangci-lint (1.62.2)
720-720: assignOp: replace
gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100)withgasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)(gocritic)
🪛 GitHub Check: Lint (ethergo)
[failure] 720-720:
assignOp: replacegasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100)withgasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)(gocritic)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/bridge/docs/06-Services/04-Submitter.md(1 hunks)ethergo/submitter/config/config.go(3 hunks)ethergo/submitter/config/config_test.go(1 hunks)ethergo/submitter/config/iconfig_generated.go(1 hunks)ethergo/submitter/submitter.go(3 hunks)ethergo/submitter/util_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ethergo/submitter/util_test.go
201-201: unnecessary leading newline
(whitespace)
207-207: unnecessary trailing newline
(whitespace)
ethergo/submitter/submitter.go
720-720: assignOp: replace gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) with gasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)
(gocritic)
🪛 GitHub Check: Lint (ethergo)
ethergo/submitter/util_test.go
[failure] 201-201:
unnecessary leading newline (whitespace)
[failure] 207-207:
unnecessary trailing newline (whitespace)
ethergo/submitter/config/config.go
[failure] 197-197:
Comment should end in a period (godot)
ethergo/submitter/submitter.go
[failure] 396-396:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 720-720:
assignOp: replace gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) with gasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100) (gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Generate (Module Changes) (ethergo)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Deploy to Vercel (docs)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
ethergo/submitter/config/config_test.go (2)
80-80: Inconsistent YAML parameter vs. assertion.
You've setdynamic_gas_unit_add_percentage: 20in the YAML, yet the test subsequently checks for a value of 5. If this is intentional (e.g., you are validating fallback logic), please clarify; otherwise, consider aligning them to avoid confusion.
88-88: Confirm the expected parameter value.
Here, the test expects a value of 5, but the YAML sets it to 20. Confirm that the fallback or override mechanism is working as intended. If not, consider adjusting the YAML or the test to ensure consistency.ethergo/submitter/submitter.go (1)
395-398: Potential integer overflow risk when converting uint64 to int.Line 396 converts
chainID.Uint64()toint, which could overflow ifchainIDis larger than the maximum integer value. Consider updatinggetGasEstimateand related config methods to accept auint64to avoid potential overflow issues.Here's a possible fix if your
getGasEstimatefunction is updated accordingly:- gasEstimate, err := t.getGasEstimate(ctx, chainClient, int(chainID.Uint64()), nil) + gasEstimate, err := t.getGasEstimate(ctx, chainClient, chainID.Uint64(), nil)🧰 Tools
🪛 GitHub Check: Lint (ethergo)
[failure] 396-396:
G115: integer overflow conversion uint64 -> int (gosec)docs/bridge/docs/06-Services/04-Submitter.md (1)
62-64: Documentation is well-explained.Thank you for adding this configuration example. It clearly communicates the purpose of
dynamic_gas_unit_add_percentage.
Deploying sanguine-fe with
|
| Latest commit: |
528839f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4c724144.sanguine-fe.pages.dev |
| Branch Preview URL: | https://gasunitbump.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ethergo/submitter/config/config_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Coverage (1.22.x, ethergo)
- GitHub Check: Deploy to Vercel (docs)
🔇 Additional comments (1)
ethergo/submitter/config/config_test.go (1)
80-80: LGTM! The new configuration parameter is properly integrated.The YAML configuration addition follows the established format and uses a reasonable test value.
| 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) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for the new configuration parameter.
While the basic YAML parsing test is good, consider adding the following test cases:
- Add a test case in
TestGettersforGetDynamicGasUnitAddPercentage - Test the default value (5%)
- 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.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
ethergo/submitter/submitter.go (2)
Line range hint
716-717: Remove debug print statementsDebug print statements should be removed before merging.
- // tmpdebug - fmt.Printf("Debug Calling EstimateGas")🧰 Tools
🪛 GitHub Check: Lint (ethergo)
[failure] 426-426:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 424-424:
var-naming: don't use underscores in Go names; var tx_forEstimate should be txForEstimate (revive)
Line range hint
723-724: Remove debug print statementsDebug print statements should be removed before merging.
- // tmpdebug - fmt.Printf("Debug Default Gas Estimate: %d\n", t.config.GetGasEstimate(chainID))🧰 Tools
🪛 GitHub Check: Lint (ethergo)
[failure] 426-426:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 424-424:
var-naming: don't use underscores in Go names; var tx_forEstimate should be txForEstimate (revive)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ethergo/submitter/submitter.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint (ethergo)
ethergo/submitter/submitter.go
[failure] 426-426:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 424-424:
var-naming: don't use underscores in Go names; var tx_forEstimate should be txForEstimate (revive)
[failure] 731-731:
assignOp: replace gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) with gasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100) (gocritic)
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 698-699: ethergo/submitter/submitter.go#L698-L699
Added lines #L698 - L699 were not covered by tests
[warning] 703-703: ethergo/submitter/submitter.go#L703
Added line #L703 was not covered by tests
[warning] 731-732: ethergo/submitter/submitter.go#L731-L732
Added lines #L731 - L732 were not covered by tests
🪛 golangci-lint (1.62.2)
ethergo/submitter/submitter.go
731-731: assignOp: replace gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) with gasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)
(gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (tools)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Coverage (1.22.x, ethergo)
- GitHub Check: Go Generate (Module Changes) (services/cctp-relayer)
- GitHub Check: Go Generate (Module Changes) (ethergo)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Generate (Module Changes) (contrib/opbot)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Deploy to Vercel (docs)
- GitHub Check: Cloudflare Pages
ethergo/submitter/submitter.go
Outdated
|
|
||
| transactor.NoSend = true | ||
|
|
||
| tx_forEstimate, err := call(transactor) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Follow Go naming conventions
The variable name tx_forEstimate uses underscores which is against Go naming conventions.
- tx_forEstimate, err := call(transactor)
+ txForEstimate, err := call(transactor)📝 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.
| tx_forEstimate, err := call(transactor) | |
| txForEstimate, err := call(transactor) |
🧰 Tools
🪛 GitHub Check: Lint (ethergo)
[failure] 424-424:
var-naming: don't use underscores in Go names; var tx_forEstimate should be txForEstimate (revive)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
ethergo/submitter/submitter.go (3)
421-423: No issues found regarding repeated NoSend assignment.
The logic of settingtransactor.NoSendto true here is consistent, and no immediate problem is evident. However, you may consider consolidating it if it's already set totrueabove to ensure clarity.
701-702: Add test coverage for dynamic gas unit percentage retrieval.
The static analyzer indicates these lines are not tested. Consider adding a dedicated test to verify different percentage values.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 701-702: ethergo/submitter/submitter.go#L701-L702
Added lines #L701 - L702 were not covered by tests
705-708: Instrument logs with test coverage.
The newly added tracer attributes, especially around the dynamic gas unit percentage, can be validated through unit or integration tests to ensure logs match expected values.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 708-708: ethergo/submitter/submitter.go#L708
Added line #L708 was not covered by testsservices/rfq/relayer/chain/chain.go (3)
77-79: Remove or adjust debug statements for production.
The//tmpdebugcomment andfmt.Printlnare typically not recommended in production. If needed, add a space after//. For better logging, consider using a logger with an appropriate level.🧰 Tools
🪛 golangci-lint (1.62.2)
77-77: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (services/rfq)
[failure] 77-77:
commentFormatting: put a space between//and comment text (gocritic)
92-94: Standardize debug logs.
Similar note here: either remove these prints or convert them into structured logs. If comments remain, add a space after//.🧰 Tools
🪛 golangci-lint (1.62.2)
92-92: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (services/rfq)
[failure] 92-92:
commentFormatting: put a space between//and comment text (gocritic)
98-100: Use a logger instead of raw prints.
Again, consider using a structured logging library. If you keep commented debug lines, ensure a space after//.🧰 Tools
🪛 golangci-lint (1.62.2)
98-98: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (services/rfq)
[failure] 98-98:
commentFormatting: put a space between//and comment text (gocritic)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ethergo/submitter/submitter.go(3 hunks)services/rfq/relayer/chain/chain.go(2 hunks)services/rfq/relayer/quoter/quoter.go(0 hunks)services/rfq/relayer/service/handlers.go(2 hunks)
💤 Files with no reviewable changes (1)
- services/rfq/relayer/quoter/quoter.go
✅ Files skipped from review due to trivial changes (1)
- services/rfq/relayer/service/handlers.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ethergo/submitter/submitter.go
[high] 429-429: G115: integer overflow conversion uint64 -> int
(gosec)
[warning] 424-424: var-naming: don't use underscores in Go names; var tx_forEstimate should be txForEstimate
(revive)
731-731: assignOp: replace gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) with gasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)
(gocritic)
services/rfq/relayer/chain/chain.go
77-77: commentFormatting: put a space between // and comment text
(gocritic)
92-92: commentFormatting: put a space between // and comment text
(gocritic)
98-98: commentFormatting: put a space between // and comment text
(gocritic)
🪛 GitHub Check: Lint (ethergo)
ethergo/submitter/submitter.go
[failure] 429-429:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 424-424:
var-naming: don't use underscores in Go names; var tx_forEstimate should be txForEstimate (revive)
[failure] 731-731:
assignOp: replace gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) with gasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100) (gocritic)
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 701-702: ethergo/submitter/submitter.go#L701-L702
Added lines #L701 - L702 were not covered by tests
[warning] 708-708: ethergo/submitter/submitter.go#L708
Added line #L708 was not covered by tests
[warning] 731-732: ethergo/submitter/submitter.go#L731-L732
Added lines #L731 - L732 were not covered by tests
🪛 GitHub Check: Lint (services/rfq)
services/rfq/relayer/chain/chain.go
[failure] 77-77:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 92-92:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 98-98:
commentFormatting: put a space between // and comment text (gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Go Coverage (1.22.x, services/omnirpc)
- GitHub Check: Go Coverage (1.22.x, services/cctp-relayer)
- GitHub Check: Go Coverage (1.22.x, ethergo)
🔇 Additional comments (7)
ethergo/submitter/submitter.go (7)
424-428: Use Go naming conventions fortx_forEstimate.
This issue was flagged in a prior review as well.- tx_forEstimate, err := call(transactor) + txForEstimate, err := call(transactor)🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 424-424: var-naming: don't use underscores in Go names; var tx_forEstimate should be txForEstimate
(revive)
🪛 GitHub Check: Lint (ethergo)
[failure] 424-424:
var-naming: don't use underscores in Go names; var tx_forEstimate should be txForEstimate (revive)
429-433: Potential integer overflow conversion when castingchainID.Uint64()toint.
This was also mentioned in a past review. Consider checking for overflow or using a larger integer type to avoid issues on 32-bit systems.🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 429-429: G115: integer overflow conversion uint64 -> int
(gosec)
🪛 GitHub Check: Lint (ethergo)
[failure] 429-429:
G115: integer overflow conversion uint64 -> int (gosec)
434-437: Gas limit assignment logic check.
Assigningtransactor.GasLimitto the newly estimated value and then togglingNoSend = falseis consistent. No new concerns here.
439-439: No additional feedback on this line.
No immediate improvements or concerns stand out.
441-441: Error handling logic is appropriate.
Returning a clear wrapped error for contract call execution aligns with good practices.
695-696: Fallback to static gas estimate.
Usingt.config.GetGasEstimate(chainID)upon disabling dynamic gas estimation is straightforward. No further issues.
730-732: Use compound assignment for clarity.
This was identified previously. Consider usinggasEstimate += gasEstimate * ...for more idiomatic Go code.🧰 Tools
🪛 golangci-lint (1.62.2)
731-731: assignOp: replace
gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100)withgasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)(gocritic)
🪛 GitHub Check: codecov/patch
[warning] 731-732: ethergo/submitter/submitter.go#L731-L732
Added lines #L731 - L732 were not covered by tests🪛 GitHub Check: Lint (ethergo)
[failure] 731-731:
assignOp: replacegasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100)withgasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)(gocritic)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/rfq/relayer/chain/chain.go(2 hunks)services/rfq/relayer/service/handlers.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/service/handlers.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
services/rfq/relayer/chain/chain.go
77-77: commentFormatting: put a space between // and comment text
(gocritic)
92-92: commentFormatting: put a space between // and comment text
(gocritic)
98-98: commentFormatting: put a space between // and comment text
(gocritic)
🪛 GitHub Check: Lint (services/rfq)
services/rfq/relayer/chain/chain.go
[failure] 77-77:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 92-92:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 98-98:
commentFormatting: put a space between // and comment text (gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Generate (Module Changes) (ethergo)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Coverage (1.22.x, services/cctp-relayer)
- GitHub Check: Go Coverage (1.22.x, ethergo)
- GitHub Check: Deploy to Vercel (docs)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
services/rfq/relayer/chain/chain.go (2)
Line range hint
77-115: Verify alignment with PR objectives.The PR's objective is to implement dynamic gas unit adjustment functionality, but this file only contains debug logging changes. Consider whether:
- The gas adjustment logic should be implemented in this file
- These debug statements are essential for the gas adjustment feature
- The changes are complete for this PR's scope
Let's check for any gas-related implementations:
✅ Verification successful
Debug statements support gas adjustment monitoring.
The debug statements in this file are essential for the gas adjustment feature as they provide visibility into:
- Transaction relay flow
- Gas amount tracking
- Transaction submission status
These logs will help monitor and debug the dynamic gas unit adjustment functionality implemented in the submitter package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for gas adjustment related code rg -g '!{vendor,node_modules}/**' -i 'dynamic.*gas.*percentage|gas.*adjustment'Length of output: 67636
🧰 Tools
🪛 golangci-lint (1.62.2)
92-92: commentFormatting: put a space between
//and comment text(gocritic)
98-98: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (services/rfq)
[failure] 92-92:
commentFormatting: put a space between//and comment text (gocritic)
[failure] 98-98:
commentFormatting: put a space between//and comment text (gocritic)
77-79:⚠️ Potential issueReplace debug print statements with structured logging.
The current implementation uses temporary
fmt.Printlnstatements for debugging. In a production service, we should use structured logging instead.Apply this diff to implement proper logging:
- //tmpdebug - fmt.Println("SubmitRelay>start: ", request.OriginTxHash) + logger := c.submitter.Logger().With(). + Str("origin_tx_hash", request.OriginTxHash). + Logger() + logger.Debug().Msg("starting relay submission") - //tmpdebug - fmt.Println("SubmitRelay>SubmitTransaction: ", request.OriginTxHash) + logger.Debug().Msg("submitting transaction") - //tmpdebug - fmt.Println("SubmitTransaction>RelayV2: ", request.OriginTxHash) + logger.Debug().Msg("executing relay v2") - //tmpdebug - fmt.Println("RelayV2 hash: ", tx.Hash()) + logger.Debug(). + Str("tx_hash", tx.Hash().String()). + Msg("relay v2 transaction submitted") - //tmpdebug - fmt.Println("SubmitRelay nonce:", nonce, "gas amount:", gasAmount) + logger.Debug(). + Uint64("nonce", nonce). + Str("gas_amount", gasAmount.String()). + Msg("relay submission completed")This change:
- Uses structured logging instead of print statements
- Adds proper context through logging fields
- Uses appropriate log levels
- Maintains better traceability
Let's verify the logger availability in the submitter interface:
Also applies to: 92-94, 98-100, 106-108, 114-115
✅ Verification successful
Replace temporary debug prints with structured logging
The review comment is correct. The codebase has an established structured logging pattern using
ipfs/go-log. The temporary debug print statements should be replaced with proper structured logging to maintain consistency and provide better observability.Evidence found in the codebase:
- Multiple components already use structured logging with proper logger initialization
- Consistent logging patterns with appropriate log levels (Debug, Warn, Error)
- Contextual information is included in log messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the submitter interface includes logging capabilities ast-grep --pattern 'type TransactionSubmitter interface { $$$ Logger() $$$ $$$ }'Length of output: 89
Script:
#!/bin/bash # Find TransactionSubmitter interface definition ast-grep --pattern 'type TransactionSubmitter interface' # Find logging-related imports rg 'import.*".*log.*"' -A 5 # Find existing structured logging patterns in the codebase rg 'logger\.' -A 2Length of output: 67611
Script:
#!/bin/bash # Find logging patterns in the relayer package rg -g 'services/rfq/relayer/**' 'logger\.' -A 2 # Find logger initialization in relayer package rg -g 'services/rfq/relayer/**' 'var logger' -A 2 # Find any logging related imports in relayer package rg -g 'services/rfq/relayer/**' 'import.*".*log.*"'Length of output: 10632
🧰 Tools
🪛 golangci-lint (1.62.2)
77-77: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (services/rfq)
[failure] 77-77:
commentFormatting: put a space between//and comment text (gocritic)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
ethergo/submitter/submitter.go (6)
391-393: Remove or convert leftover debug statements and fix comment style
It’s generally best practice to remove or replace debug statements like//tmpdebugwith structured logs. Also, add a space after//to match Go comment guidelines.- //tmpdebug + // tmpdebug fmt.Printf("SubmitTransaction>setGasPrice")🧰 Tools
🪛 golangci-lint (1.62.2)
391-391: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (ethergo)
[failure] 391-391:
commentFormatting: put a space between//and comment text (gocritic)
425-427: Remove or convert leftover debug statements and fix comment style
Same note as above: consider removing or adapting debug prints and use a space after//.- //tmpdebug + // tmpdebug fmt.Printf("SubmitTransaction>call")🧰 Tools
🪛 golangci-lint (1.62.2)
425-425: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (ethergo)
[failure] 425-425:
commentFormatting: put a space between//and comment text (gocritic)
435-437: Remove or convert leftover debug statements and fix comment style
Again, these debugging logs can be switched to structured logging or removed.- //tmpdebug + // tmpdebug fmt.Printf("Gas estimate: %d\n", gasEstimate)🧰 Tools
🪛 golangci-lint (1.62.2)
435-435: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (ethergo)
[failure] 435-435:
commentFormatting: put a space between//and comment text (gocritic)
457-459: Remove or convert leftover debug statements and fix comment style
Same debug/logging considerations as previous comments.- //tmpdebug + // tmpdebug fmt.Printf("SubmitTransaction>tx.Nonce: %d\n", tx.Nonce())
707-708: Add test coverage for dynamic gas unit percentage
Lines setting thegasUnitAddPercentagearen’t tested according to coverage reports. Add a small unit or integration test to confirm correct handling under different configurations.// Example test snippet func TestDynamicGasUnitAddPercentage(t *testing.T) { // ... }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 707-708: ethergo/submitter/submitter.go#L707-L708
Added lines #L707 - L708 were not covered by tests
736-738: Use+=operator and consider overflow checks
Replace the verbose addition expression with+=. Also consider adding a guard for potential overflow ifgasEstimateis large.- gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) + gasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)🧰 Tools
🪛 golangci-lint (1.62.2)
737-737: assignOp: replace
gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100)withgasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)(gocritic)
🪛 GitHub Check: codecov/patch
[warning] 737-738: ethergo/submitter/submitter.go#L737-L738
Added lines #L737 - L738 were not covered by tests🪛 GitHub Check: Lint (ethergo)
[failure] 737-737:
assignOp: replacegasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100)withgasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)(gocritic)services/rfq/relayer/chain/chain.go (5)
77-79: Refine debug statements and fix comment style
Consider removing or converting this to structured logs. Also, ensure a space after//for Go style.- //tmpdebug + // tmpdebug fmt.Println("SubmitRelay>start: ", request.OriginTxHash)🧰 Tools
🪛 golangci-lint (1.62.2)
77-77: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (services/rfq)
[failure] 77-77:
commentFormatting: put a space between//and comment text (gocritic)🪛 GitHub Actions: Go Workflows
[error] 77-77: commentFormatting: put a space between
//and comment text (gocritic)
92-94: Refine debug statements and fix comment style
Same as above. Apply the same approach to unify comment formatting.- //tmpdebug + // tmpdebug fmt.Println("SubmitRelay>SubmitTransaction: ", request.OriginTxHash)🧰 Tools
🪛 golangci-lint (1.62.2)
92-92: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (services/rfq)
[failure] 92-92:
commentFormatting: put a space between//and comment text (gocritic)
98-100: Refine debug statements and fix comment style
Similarly, remove or update the debug prints and correct the comment spacing.- //tmpdebug + // tmpdebug fmt.Println("SubmitTransaction>RelayV2: ", request.OriginTxHash)🧰 Tools
🪛 golangci-lint (1.62.2)
98-98: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (services/rfq)
[failure] 98-98:
commentFormatting: put a space between//and comment text (gocritic)
106-108: Refine debug statements and fix comment style
Again, consider removing temporary debug lines and standardizing the comment format.- //tmpdebug + // tmpdebug fmt.Printf("RelayV2 transaction: %+v\n", tx)
114-115: Refine debug statements and fix comment style
As with previous segments, convert this debug usage to a more permanent logging approach or remove if no longer needed.- //tmpdebug + // tmpdebug fmt.Println("SubmitRelay nonce:", nonce, "gas amount:", gasAmount)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ethergo/submitter/submitter.go(5 hunks)services/rfq/relayer/chain/chain.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ethergo/submitter/submitter.go
391-391: commentFormatting: put a space between // and comment text
(gocritic)
425-425: commentFormatting: put a space between // and comment text
(gocritic)
435-435: commentFormatting: put a space between // and comment text
(gocritic)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
737-737: assignOp: replace gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) with gasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100)
(gocritic)
services/rfq/relayer/chain/chain.go
77-77: commentFormatting: put a space between // and comment text
(gocritic)
92-92: commentFormatting: put a space between // and comment text
(gocritic)
98-98: commentFormatting: put a space between // and comment text
(gocritic)
🪛 GitHub Check: Lint (ethergo)
ethergo/submitter/submitter.go
[failure] 391-391:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 425-425:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 435-435:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 737-737:
assignOp: replace gasEstimate = gasEstimate + (gasEstimate * uint64(gasUnitAddPercentage) / 100) with gasEstimate += (gasEstimate * uint64(gasUnitAddPercentage) / 100) (gocritic)
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 707-708: ethergo/submitter/submitter.go#L707-L708
Added lines #L707 - L708 were not covered by tests
[warning] 714-714: ethergo/submitter/submitter.go#L714
Added line #L714 was not covered by tests
[warning] 737-738: ethergo/submitter/submitter.go#L737-L738
Added lines #L737 - L738 were not covered by tests
🪛 GitHub Actions: Go Workflows
ethergo/submitter/submitter.go
[error] 443-443: Nil pointer dereference in txSubmitterImpl.SubmitTransaction method
services/rfq/relayer/chain/chain.go
[error] 77-77: commentFormatting: put a space between // and comment text (gocritic)
🪛 GitHub Check: Lint (services/rfq)
services/rfq/relayer/chain/chain.go
[failure] 77-77:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 92-92:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 98-98:
commentFormatting: put a space between // and comment text (gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: run-goreleaser (tools)
- GitHub Check: run-goreleaser (ethergo)
- GitHub Check: run-goreleaser (agents)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
ethergo/submitter/submitter.go (5)
438-441: Confirm error handling logic post gas limit assignment
After settingtransactor.GasLimit, theif err != nilcheck uses the sameerrvariable without additional changes in between. Consider ensuring correct error validation from any subsequent call or clarifying the logic to avoid confusion.
701-702: No issues found
This comment line seems fine.
711-714: Good logging addition
StoringgasUnitAddPercentagein trace attributes helps debugging. This is a good addition.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 714-714: ethergo/submitter/submitter.go#L714
Added line #L714 was not covered by tests
445-447:⚠️ Potential issueAvoid nil pointer dereference when unlocking
If the signer is never invoked,lockerremains nil. This causes a nil pointer dereference atlocker.Unlock(). Confirm that the contract call always triggers the signer, or defensively check iflockeris nil.defer func() { - locker.Unlock() + if locker != nil { + locker.Unlock() + } }()Likely invalid or redundant comment.
430-433:⚠️ Potential issuePrevent potential integer overflow
ConvertingchainID.Uint64()directly tointcan overflow on 32-bit systems if the value exceedsmath.MaxInt32.- gasEstimate, err := t.getGasEstimate(ctx, chainClient, int(chainID.Uint64()), tx) + chainIDInt := chainID.Int64() + if chainIDInt > math.MaxInt32 { + return 0, fmt.Errorf("chain ID %d exceeds maximum supported value", chainIDInt) + } + gasEstimate, err := t.getGasEstimate(ctx, chainClient, int(chainIDInt), tx)Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
🪛 GitHub Check: Lint (ethergo)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
ethergo/submitter/submitter.go (1)
Fix gas calculation overflow risk and follow consistent naming
The review identified two valid concerns:
- The gas limit calculation
gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100)is susceptible to overflow since it uses uint64 arithmetic without checks- Variable names with underscores don't follow the codebase's utility code convention of using camelCase
The suggestion to use big.Int is appropriate as it matches the pattern seen in other gas-related calculations throughout the codebase.
🔗 Analysis chain
Line range hint
730-791: Fix variable naming and add overflow protection.The gas estimation function has several issues:
- Variable names use underscores against Go conventions
- Potential integer overflow in gas limit calculations
- Missing test coverage for critical functionality
Apply this diff to fix the issues:
-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) { - gasLimit_fromPrior := tx.Gas() - gasLimit_fromEstimate, err := chainClient.EstimateGas(ctx, *call) + gasLimitFromPrior := tx.Gas() + gasLimitFromEstimate, err := chainClient.EstimateGas(ctx, *call) - gasLimit_new = max(gasLimit_fromPrior, gasLimit_fromEstimate) + gasLimitNew = max(gasLimitFromPrior, gasLimitFromEstimate) - gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100) + // Protect against overflow in gas limit calculation + 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()Run this script to verify test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test coverage for gas estimation go test -coverprofile=coverage.out ./ethergo/submitter/ go tool cover -func=coverage.out | grep getGasEstimateLength of output: 12924
Script:
#!/bin/bash # Check variable naming patterns and gas calculations echo "=== Variable naming patterns ===" rg "gasLimit[A-Z]" --type go echo -e "\n=== Gas calculation patterns ===" rg "gas.*\*.*\/\s*100" --type go -A 2 -B 2 echo -e "\n=== Gas estimation function implementations ===" ast-grep --pattern 'func $$$getGasEstimate$$$'Length of output: 968
Script:
#!/bin/bash # Search with corrected patterns echo "=== Gas calculations ===" rg "gasLimit.*\+" --type go -A 2 -B 2 echo -e "\n=== Big.Int usage in gas calculations ===" rg "big\.Int.*gas" --type go -A 2 -B 2 echo -e "\n=== Gas estimation implementations ===" rg "func.*getGasEstimate" --type go -A 5Length of output: 124755
🧰 Tools
🪛 golangci-lint (1.62.2)
784-784: assignOp: replace
gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100)withgasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100)(gocritic)
[high] 786-786: G115: integer overflow conversion uint64 -> int64
(gosec)
[high] 787-787: G115: integer overflow conversion uint64 -> int64
(gosec)
[high] 788-788: G115: integer overflow conversion uint64 -> int64
(gosec)
[warning] 765-765: var-naming: don't use underscores in Go names; var gasLimit_fromPrior should be gasLimitFromPrior
(revive)
[warning] 767-767: var-naming: don't use underscores in Go names; var gasLimit_fromEstimate should be gasLimitFromEstimate
(revive)
🪛 GitHub Check: codecov/patch
[warning] 771-772: ethergo/submitter/submitter.go#L771-L772
Added lines #L771 - L772 were not covered by tests
[warning] 774-775: ethergo/submitter/submitter.go#L774-L775
Added lines #L774 - L775 were not covered by tests
[warning] 782-782: ethergo/submitter/submitter.go#L782
Added line #L782 was not covered by tests
[warning] 790-790: ethergo/submitter/submitter.go#L790
Added line #L790 was not covered by tests🪛 GitHub Check: Lint (ethergo)
[failure] 784-784:
assignOp: replacegasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100)withgasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100)(gocritic)
[failure] 786-786:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 787-787:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 788-788:
G115: integer overflow conversion uint64 -> int64 (gosec)
♻️ Duplicate comments (3)
ethergo/submitter/submitter.go (3)
391-392: 🛠️ Refactor suggestionReplace debug prints with structured logging.
Replace temporary debug print statements with structured logging using the existing
loggerinstance for better observability and consistency.Apply this diff to improve logging:
- //tmpdebug - fmt.Printf("SubmitTransaction>setGasPrice\n") + logger.Debug("submitting transaction: setting gas price") - //tmpdebug - fmt.Printf("test ver 7\n") + logger.Debug("submitting transaction: version 7") - //tmpdebug - fmt.Printf("Using Default \n") + logger.Debug("using default gas estimate") - //tmpdebug - fmt.Printf("transactor.GasLimit: %d\n", transactor.GasLimit) + logger.Debugw("gas limit set", "limit", transactor.GasLimit) - //tmpdebug - fmt.Printf("tx.Gas: %d\n", tx.Gas()) + logger.Debugw("transaction gas", "gas", tx.Gas()) - //tmpdebug - fmt.Printf("SubmitTransaction>storeTX\n") + logger.Debug("submitting transaction: storing tx") - //tmpdebug - fmt.Printf("SubmitTransaction>tx.Nonce: %d\n", tx.Nonce()) + logger.Debugw("transaction nonce", "nonce", tx.Nonce()) - //tmpdebug - fmt.Println("getGasEstimate>start") + logger.Debug("starting gas estimation") - //tmpdebug - fmt.Println("getGasEstimate>gasUnitAddPercentage", gasUnitAddPercentage) + logger.Debugw("gas unit add percentage", "percentage", gasUnitAddPercentage) - //tmpdebug - fmt.Printf("getGasEstimate> Error estimating gas: %v\n", err) + logger.Errorw("failed to estimate gas", "error", err)Also applies to: 425-426, 432-433, 462-463, 470-471, 475-476, 487-488, 737-738, 742-743, 771-772
🧰 Tools
🪛 golangci-lint (1.62.2)
391-391: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (ethergo)
[failure] 391-391:
commentFormatting: put a space between//and comment text (gocritic)
438-449:⚠️ Potential issueRestore gas estimation logic.
The gas estimation logic is commented out, which is critical for proper transaction handling. This code should either be restored or safely removed if it's no longer needed.
Uncomment and restore the gas estimation logic if it's still needed:
- // //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) - // } + transactorForGasEstimate := copyTransactOpts(transactor) + txForGasEstimate, err := call(transactorForGasEstimate) + if err != nil { + return 0, fmt.Errorf("err contract call for gas est: %w", err) + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 440-440: ethergo/submitter/submitter.go#L440
Added line #L440 was not covered by tests
[warning] 442-442: ethergo/submitter/submitter.go#L442
Added line #L442 was not covered by tests
[warning] 444-444: ethergo/submitter/submitter.go#L444
Added line #L444 was not covered by tests
430-430:⚠️ Potential issuePrevent potential integer overflow in chain ID conversion.
The conversion of
chainID.Uint64()tointcould overflow on 32-bit systems.Apply this diff to fix the issue:
+ chainIDInt := chainID.Int64() + if chainIDInt > math.MaxInt32 { + return 0, fmt.Errorf("chain ID %d exceeds maximum supported value", chainIDInt) + } - if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) { + if !t.config.GetDynamicGasEstimate(int(chainIDInt)) { - transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) + transactor.GasLimit = t.config.GetGasEstimate(int(chainIDInt))Also applies to: 435-435
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
🪛 GitHub Check: Lint (ethergo)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ethergo/submitter/submitter.go(5 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ethergo/submitter/submitter.go
391-391: commentFormatting: put a space between // and comment text
(gocritic)
425-425: commentFormatting: put a space between // and comment text
(gocritic)
432-432: commentFormatting: put a space between // and comment text
(gocritic)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 435-435: G115: integer overflow conversion uint64 -> int
(gosec)
436-436: unnecessary leading newline
(whitespace)
[warning] 730-730: var-naming: don't use underscores in Go names; method result gasLimit_new should be gasLimitNew
(revive)
784-784: assignOp: replace gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100) with gasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100)
(gocritic)
[high] 786-786: G115: integer overflow conversion uint64 -> int64
(gosec)
[high] 787-787: G115: integer overflow conversion uint64 -> int64
(gosec)
[high] 788-788: G115: integer overflow conversion uint64 -> int64
(gosec)
[warning] 765-765: var-naming: don't use underscores in Go names; var gasLimit_fromPrior should be gasLimitFromPrior
(revive)
[warning] 767-767: var-naming: don't use underscores in Go names; var gasLimit_fromEstimate should be gasLimitFromEstimate
(revive)
🪛 GitHub Check: Lint (ethergo)
ethergo/submitter/submitter.go
[failure] 391-391:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 425-425:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 432-432:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 435-435:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 784-784:
assignOp: replace gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100) with gasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100) (gocritic)
[failure] 786-786:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 787-787:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 788-788:
G115: integer overflow conversion uint64 -> int64 (gosec)
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 437-437: ethergo/submitter/submitter.go#L437
Added line #L437 was not covered by tests
[warning] 440-440: ethergo/submitter/submitter.go#L440
Added line #L440 was not covered by tests
[warning] 442-442: ethergo/submitter/submitter.go#L442
Added line #L442 was not covered by tests
[warning] 444-444: ethergo/submitter/submitter.go#L444
Added line #L444 was not covered by tests
[warning] 451-451: ethergo/submitter/submitter.go#L451
Added line #L451 was not covered by tests
[warning] 467-467: ethergo/submitter/submitter.go#L467
Added line #L467 was not covered by tests
[warning] 731-732: ethergo/submitter/submitter.go#L731-L732
Added lines #L731 - L732 were not covered by tests
[warning] 738-744: ethergo/submitter/submitter.go#L738-L744
Added lines #L738 - L744 were not covered by tests
[warning] 747-747: ethergo/submitter/submitter.go#L747
Added line #L747 was not covered by tests
[warning] 749-750: ethergo/submitter/submitter.go#L749-L750
Added lines #L749 - L750 were not covered by tests
[warning] 771-772: ethergo/submitter/submitter.go#L771-L772
Added lines #L771 - L772 were not covered by tests
[warning] 774-775: ethergo/submitter/submitter.go#L774-L775
Added lines #L774 - L775 were not covered by tests
[warning] 782-782: ethergo/submitter/submitter.go#L782
Added line #L782 was not covered by tests
[warning] 790-790: ethergo/submitter/submitter.go#L790
Added line #L790 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Go Generate (Module Changes) (tools)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/cctp-relayer)
- GitHub Check: Go Coverage (1.22.x, ethergo)
- GitHub Check: Go Generate (Module Changes) (ethergo)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Generate (Module Changes) (contrib/opbot)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Build (1.22.x)
- GitHub Check: Deploy to Vercel (docs)
- GitHub Check: Cloudflare Pages
ethergo/submitter/submitter.go
Outdated
| // spoof some low gas units to induce bump tests | ||
| if chainID.Uint64() == 10 { | ||
| transactor.GasLimit = 25000 | ||
| } else if chainID.Uint64() == 42161 { | ||
| transactor.GasLimit = 150000 | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove hardcoded gas limits used for testing.
The code contains hardcoded gas limits for specific chain IDs (10 and 42161) with the comment "spoof some low gas units to induce bump tests". This is dangerous in production as:
- Low gas limits can cause transaction failures
- Different transactions require different gas limits
- Gas requirements can change over time
Either:
- Remove the hardcoded values and use proper gas estimation, or
- Move this testing logic behind a configuration flag
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ethergo/submitter/submitter.go(5 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ethergo/submitter/submitter.go
391-391: commentFormatting: put a space between // and comment text
(gocritic)
425-425: commentFormatting: put a space between // and comment text
(gocritic)
432-432: commentFormatting: put a space between // and comment text
(gocritic)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 435-435: G115: integer overflow conversion uint64 -> int
(gosec)
436-436: unnecessary leading newline
(whitespace)
[warning] 730-730: var-naming: don't use underscores in Go names; method result gasLimit_new should be gasLimitNew
(revive)
784-784: assignOp: replace gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100) with gasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100)
(gocritic)
[high] 786-786: G115: integer overflow conversion uint64 -> int64
(gosec)
[high] 787-787: G115: integer overflow conversion uint64 -> int64
(gosec)
[high] 788-788: G115: integer overflow conversion uint64 -> int64
(gosec)
[warning] 765-765: var-naming: don't use underscores in Go names; var gasLimit_fromPrior should be gasLimitFromPrior
(revive)
[warning] 767-767: var-naming: don't use underscores in Go names; var gasLimit_fromEstimate should be gasLimitFromEstimate
(revive)
🪛 GitHub Check: Lint (ethergo)
ethergo/submitter/submitter.go
[failure] 391-391:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 425-425:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 432-432:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 435-435:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 784-784:
assignOp: replace gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100) with gasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100) (gocritic)
[failure] 786-786:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 787-787:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 788-788:
G115: integer overflow conversion uint64 -> int64 (gosec)
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 437-453: ethergo/submitter/submitter.go#L437-L453
Added lines #L437 - L453 were not covered by tests
[warning] 455-455: ethergo/submitter/submitter.go#L455
Added line #L455 was not covered by tests
[warning] 457-457: ethergo/submitter/submitter.go#L457
Added line #L457 was not covered by tests
[warning] 459-459: ethergo/submitter/submitter.go#L459
Added line #L459 was not covered by tests
[warning] 467-467: ethergo/submitter/submitter.go#L467
Added line #L467 was not covered by tests
[warning] 730-732: ethergo/submitter/submitter.go#L730-L732
Added lines #L730 - L732 were not covered by tests
[warning] 738-744: ethergo/submitter/submitter.go#L738-L744
Added lines #L738 - L744 were not covered by tests
[warning] 747-747: ethergo/submitter/submitter.go#L747
Added line #L747 was not covered by tests
[warning] 749-750: ethergo/submitter/submitter.go#L749-L750
Added lines #L749 - L750 were not covered by tests
[warning] 754-754: ethergo/submitter/submitter.go#L754
Added line #L754 was not covered by tests
[warning] 765-768: ethergo/submitter/submitter.go#L765-L768
Added lines #L765 - L768 were not covered by tests
[warning] 771-772: ethergo/submitter/submitter.go#L771-L772
Added lines #L771 - L772 were not covered by tests
[warning] 774-777: ethergo/submitter/submitter.go#L774-L777
Added lines #L774 - L777 were not covered by tests
[warning] 781-791: ethergo/submitter/submitter.go#L781-L791
Added lines #L781 - L791 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Coverage (1.22.x, contrib/opbot)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Generate (Module Changes) (ethergo)
- GitHub Check: Go Generate (Module Changes) (contrib/opbot)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Deploy to Vercel (docs)
🔇 Additional comments (5)
ethergo/submitter/submitter.go (5)
391-392: Replace debug prints with structured logging.Replace temporary debug print statements with structured logging using the existing
loggerinstance.Apply this diff to improve logging:
- //tmpdebug - fmt.Printf("SubmitTransaction>setGasPrice\n") + logger.Debug("submitting transaction: setting gas price") - //tmpdebug - fmt.Printf("test ver 7\n") + logger.Debug("processing transaction v7") - //tmpdebug - fmt.Printf("Using Default \n") + logger.Debug("using default gas estimate") - //tmpdebug - fmt.Printf("transactor.GasLimit: %d\n", transactor.GasLimit) + logger.Debugf("gas limit set to: %d", transactor.GasLimit) - //tmpdebug - fmt.Printf("tx.Gas: %d\n", tx.Gas()) + logger.Debugf("transaction gas: %d", tx.Gas()) - //tmpdebug - fmt.Printf("SubmitTransaction>storeTX\n") + logger.Debug("storing transaction") - //tmpdebug - fmt.Printf("SubmitTransaction>tx.Nonce: %d\n", tx.Nonce()) + logger.Debugf("transaction nonce: %d", tx.Nonce()) - //tmpdebug - fmt.Println("getGasEstimate>start") + logger.Debug("starting gas estimation") - //tmpdebug - fmt.Println("getGasEstimate>gasUnitAddPercentage", gasUnitAddPercentage) + logger.Debugf("gas unit add percentage: %d", gasUnitAddPercentage) - //tmpdebug - fmt.Printf("getGasEstimate> Error estimating gas: %v\n", err) + logger.Errorf("failed to estimate gas: %v", err)Also applies to: 425-426, 432-433, 462-463, 470-471, 475-476, 487-488, 737-738, 742-743, 771-772
🧰 Tools
🪛 golangci-lint (1.62.2)
391-391: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (ethergo)
[failure] 391-391:
commentFormatting: put a space between//and comment text (gocritic)
730-730: Follow Go naming conventions.Several variables use underscores which is against Go naming conventions:
gasLimit_newgasLimit_fromPriorgasLimit_fromEstimateApply this diff to fix the naming:
-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) { - gasLimit_fromPrior := tx.Gas() + gasLimitFromPrior := tx.Gas() - gasLimit_fromEstimate, err := chainClient.EstimateGas(ctx, *call) + gasLimitFromEstimate, err := chainClient.EstimateGas(ctx, *call) - gasLimit_new = max(gasLimit_fromPrior, gasLimit_fromEstimate) + gasLimitNew = max(gasLimitFromPrior, gasLimitFromEstimate) - gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100) + gasLimitNew += gasLimitNew * uint64(gasUnitAddPercentage) / 100Also applies to: 765-765, 767-767, 781-781, 784-784
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 730-730: var-naming: don't use underscores in Go names; method result gasLimit_new should be gasLimitNew
(revive)
🪛 GitHub Check: codecov/patch
[warning] 730-732: ethergo/submitter/submitter.go#L730-L732
Added lines #L730 - L732 were not covered by tests
430-430: Prevent potential integer overflow.Several uint64 to int/int64 conversions could potentially overflow:
- Chain ID conversions in
GetDynamicGasEstimateandGetGasEstimatecalls- Gas limit conversions in trace attributes
Apply this diff to add overflow protection:
+ chainIDInt := chainID.Int64() + if chainIDInt > math.MaxInt32 { + return 0, fmt.Errorf("chain ID %d exceeds maximum supported value", chainIDInt) + } - if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) { - transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) + if !t.config.GetDynamicGasEstimate(int(chainIDInt)) { + transactor.GasLimit = t.config.GetGasEstimate(int(chainIDInt)) - 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.Uint64("gas_limit", gasLimitNew), + attribute.Uint64("gas_limit_from_prior", gasLimitFromPrior), + attribute.Uint64("gas_limit_from_estimate", gasLimitFromEstimate),Also applies to: 435-435, 786-789
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
🪛 GitHub Check: Lint (ethergo)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
Line range hint
730-791: Add test coverage for dynamic gas unit percentage adjustment.The new gas estimation logic lacks test coverage, particularly:
- Dynamic gas unit percentage calculation
- Gas limit adjustment based on prior and estimated values
- Error handling in gas estimation
Run this script to verify test coverage:
#!/bin/bash # Check test coverage for the new gas estimation logic go test -coverprofile=coverage.out ./ethergo/submitter/ go tool cover -func=coverage.out | grep -A 2 "getGasEstimate"🧰 Tools
🪛 golangci-lint (1.62.2)
784-784: assignOp: replace
gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100)withgasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100)(gocritic)
[high] 786-786: G115: integer overflow conversion uint64 -> int64
(gosec)
[high] 787-787: G115: integer overflow conversion uint64 -> int64
(gosec)
[high] 788-788: G115: integer overflow conversion uint64 -> int64
(gosec)
[warning] 765-765: var-naming: don't use underscores in Go names; var gasLimit_fromPrior should be gasLimitFromPrior
(revive)
[warning] 767-767: var-naming: don't use underscores in Go names; var gasLimit_fromEstimate should be gasLimitFromEstimate
(revive)
🪛 GitHub Check: codecov/patch
[warning] 765-768: ethergo/submitter/submitter.go#L765-L768
Added lines #L765 - L768 were not covered by tests
[warning] 771-772: ethergo/submitter/submitter.go#L771-L772
Added lines #L771 - L772 were not covered by tests
[warning] 774-777: ethergo/submitter/submitter.go#L774-L777
Added lines #L774 - L777 were not covered by tests
[warning] 781-791: ethergo/submitter/submitter.go#L781-L791
Added lines #L781 - L791 were not covered by tests🪛 GitHub Check: Lint (ethergo)
[failure] 784-784:
assignOp: replacegasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100)withgasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100)(gocritic)
[failure] 786-786:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 787-787:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 788-788:
G115: integer overflow conversion uint64 -> int64 (gosec)
437-453:⚠️ Potential issueRestore commented-out gas estimation logic.
The gas estimation logic is commented out, which could lead to incorrect gas limits being used.
Uncomment and restore the gas estimation logic:
- // //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 + transactorForGasEstimate := copyTransactOpts(transactor) + txForGasEstimate, err := call(transactorForGasEstimate) + if err != nil { + return 0, fmt.Errorf("err contract call for gas est: %w", err) + } + + gasEstimate, err := t.getGasEstimate(ctx, chainClient, int(chainID.Uint64()), txForGasEstimate) + if err != nil { + return 0, fmt.Errorf("err getGasEstimate: %w", err) + } + transactor.GasLimit = gasEstimateLikely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 437-453: ethergo/submitter/submitter.go#L437-L453
Added lines #L437 - L453 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
ethergo/submitter/submitter.go (1)
Add test coverage for gas estimation logic
The review comment is valid. The codebase lacks test coverage for the gas estimation functionality. Critical paths that need test coverage:
- Dynamic gas estimation toggle behavior
- Error handling with fallback to default gas
- Gas limit percentage adjustments
- Integration with metrics/tracing
The implementation in
getGasEstimatehandles complex scenarios and configuration options, making test coverage essential for reliability and maintainability.🔗 Analysis chain
Line range hint
735-796: Add test coverage for gas estimation logic.The new gas estimation logic lacks test coverage, particularly:
- Dynamic gas estimation functionality
- Gas estimation error handling
- Gas limit percentage adjustment
Run this script to verify test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test coverage for the new gas estimation logic go test -coverprofile=coverage.out ./ethergo/submitter/ go tool cover -func=coverage.out | grep -A 2 "getGasEstimate"Length of output: 13647
Script:
#!/bin/bash # Search for test files and gas estimation related tests echo "=== Test files in submitter directory ===" fd ".*_test\.go$" ethergo/submitter/ echo -e "\n=== Gas estimation related test cases ===" rg -A 5 "Test.*Gas.*Estimate" ethergo/submitter/ rg -A 5 "getGasEstimate" ethergo/submitter/ --type goLength of output: 3148
🧰 Tools
🪛 golangci-lint (1.62.2)
789-789: assignOp: replace
gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100)withgasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100)(gocritic)
[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)
[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
Added lines #L770 - L773 were not covered by tests
[warning] 776-777: ethergo/submitter/submitter.go#L776-L777
Added lines #L776 - L777 were not covered by tests
[warning] 779-782: ethergo/submitter/submitter.go#L779-L782
Added lines #L779 - L782 were not covered by tests
[warning] 786-796: ethergo/submitter/submitter.go#L786-L796
Added lines #L786 - L796 were not covered by tests🪛 GitHub Check: Lint (ethergo)
[failure] 789-789:
assignOp: replacegasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100)withgasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100)(gocritic)
[failure] 791-791:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 792-792:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 793-793:
G115: integer overflow conversion uint64 -> int64 (gosec)
🧹 Nitpick comments (1)
.gitignore (1)
138-138: Consider documenting the config structure.Adding
config-stage.yamlto .gitignore is appropriate for environment-specific configurations. However, consider adding aconfig-stage.yaml.exampletemplate to the repository to document the expected structure and required fields, including the newdynamic_gas_unit_add_percentageparameter.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)ethergo/submitter/submitter.go(5 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ethergo/submitter/submitter.go
391-391: commentFormatting: put a space between // and comment text
(gocritic)
425-425: commentFormatting: put a space between // and comment text
(gocritic)
432-432: commentFormatting: put a space between // and comment text
(gocritic)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 435-435: G115: integer overflow conversion uint64 -> int
(gosec)
436-436: unnecessary leading newline
(whitespace)
[warning] 735-735: var-naming: don't use underscores in Go names; method result gasLimit_new should be gasLimitNew
(revive)
789-789: assignOp: replace gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100) with gasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100)
(gocritic)
[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)
[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: Lint (ethergo)
ethergo/submitter/submitter.go
[failure] 391-391:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 425-425:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 432-432:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 435-435:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 789-789:
assignOp: replace gasLimit_new = gasLimit_new + (gasLimit_new * uint64(gasUnitAddPercentage) / 100) with gasLimit_new += (gasLimit_new * uint64(gasUnitAddPercentage) / 100) (gocritic)
[failure] 791-791:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 792-792:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 793-793:
G115: integer overflow conversion uint64 -> int64 (gosec)
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 437-459: ethergo/submitter/submitter.go#L437-L459
Added lines #L437 - L459 were not covered by tests
[warning] 472-472: ethergo/submitter/submitter.go#L472
Added line #L472 was not covered by tests
[warning] 735-737: ethergo/submitter/submitter.go#L735-L737
Added lines #L735 - L737 were not covered by tests
[warning] 743-749: ethergo/submitter/submitter.go#L743-L749
Added lines #L743 - L749 were not covered by tests
[warning] 752-752: ethergo/submitter/submitter.go#L752
Added line #L752 was not covered by tests
[warning] 754-755: ethergo/submitter/submitter.go#L754-L755
Added lines #L754 - L755 were not covered by tests
[warning] 759-759: ethergo/submitter/submitter.go#L759
Added line #L759 was not covered by tests
[warning] 770-773: ethergo/submitter/submitter.go#L770-L773
Added lines #L770 - L773 were not covered by tests
[warning] 776-777: ethergo/submitter/submitter.go#L776-L777
Added lines #L776 - L777 were not covered by tests
[warning] 779-782: ethergo/submitter/submitter.go#L779-L782
Added lines #L779 - L782 were not covered by tests
[warning] 786-796: ethergo/submitter/submitter.go#L786-L796
Added lines #L786 - L796 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Go Generate (Module Changes) (tools)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/cctp-relayer)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (ethergo)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Generate (Module Changes) (contrib/opbot)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Deploy to Vercel (docs)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
.gitignore (1)
137-137: LGTM! Good security practice.Adding
*signer*.txtto .gitignore is a good security practice to prevent accidentally committing files that might contain sensitive signer-related information.ethergo/submitter/submitter.go (2)
Line range hint
391-494: Replace debug prints with structured logging.Replace temporary debug print statements with structured logging using the existing
loggerinstance.🧰 Tools
🪛 golangci-lint (1.62.2)
425-425: commentFormatting: put a space between
//and comment text(gocritic)
432-432: commentFormatting: put a space between
//and comment text(gocritic)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 435-435: G115: integer overflow conversion uint64 -> int
(gosec)
436-436: unnecessary leading newline
(whitespace)
🪛 GitHub Check: codecov/patch
[warning] 437-459: ethergo/submitter/submitter.go#L437-L459
Added lines #L437 - L459 were not covered by tests
[warning] 472-472: ethergo/submitter/submitter.go#L472
Added line #L472 was not covered by tests🪛 GitHub Check: Lint (ethergo)
[failure] 425-425:
commentFormatting: put a space between//and comment text (gocritic)
[failure] 432-432:
commentFormatting: put a space between//and comment text (gocritic)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 435-435:
G115: integer overflow conversion uint64 -> int (gosec)
454-460: 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:
- Will cause transaction failures with zero gas limit
- May lead to out-of-gas errors with insufficient gas limit
- Wastes gas if the limit is too high
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
services/rfq/contracts/fastbridgev2/events.go (1)
Line range hint
28-65: Initialize BridgeQuoteDetailsTopic in init function.The new topic needs to be initialized and verified like other topics.
func init() { // ... existing code ... BridgeDepositClaimedTopic = parsedABI.Events["BridgeDepositClaimed"].ID BridgeProofDisputedTopic = parsedABI.Events["BridgeProofDisputed"].ID + BridgeQuoteDetailsTopic = parsedABI.Events["BridgeQuoteDetails"].ID // ... existing verifications ... _, err = parsedABI.EventByID(BridgeProofDisputedTopic) if err != nil { panic(err) } + _, err = parsedABI.EventByID(BridgeQuoteDetailsTopic) + if err != nil { + panic(err) + } }
🧹 Nitpick comments (2)
services/rfq/contracts/fastbridgev2/parser.go (1)
26-27: Add missing period in comment.Fix the comment to follow the project's style guide.
- // BridgeQuoteDetailsEvent is emitted along w/ BridgeRequestedEvent as supplemental data + // BridgeQuoteDetailsEvent is emitted along w/ BridgeRequestedEvent as supplemental data.🧰 Tools
🪛 golangci-lint (1.62.2)
26-26: Comment should end in a period
(godot)
🪛 GitHub Check: Lint (services/rfq)
[failure] 26-26:
Comment should end in a period (godot)services/rfq/relayer/chain/chain.go (1)
93-93: Remove unnecessary blank line.The blank line at line 93 is unnecessary and can be removed.
Apply this diff to remove the unnecessary blank line:
-
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ethergo/submitter/submitter.go(4 hunks)services/rfq/contracts/fastbridgev2/events.go(2 hunks)services/rfq/contracts/fastbridgev2/eventtype_string.go(1 hunks)services/rfq/contracts/fastbridgev2/parser.go(2 hunks)services/rfq/relayer/chain/chain.go(1 hunks)services/rfq/relayer/pricer/fee_pricer.go(1 hunks)services/rfq/relayer/service/handlers.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/service/handlers.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
services/rfq/contracts/fastbridgev2/parser.go
26-26: Comment should end in a period
(godot)
ethergo/submitter/submitter.go
[high] 425-425: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 465-465: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 466-466: G115: integer overflow conversion int -> uint64
(gosec)
[high] 426-426: G115: integer overflow conversion uint64 -> int
(gosec)
[warning] 430-430: var-naming: don't use underscores in Go names; var transactor_forGasEstimate should be transactorForGasEstimate
(revive)
[warning] 452-452: var-naming: don't use underscores in Go names; var tx_forGasEstimate should be txForGasEstimate
(revive)
427-427: unnecessary leading newline
(whitespace)
434-434: unnecessary leading newline
(whitespace)
[high] 741-741: G115: integer overflow conversion uint64 -> int64
(gosec)
725-725: unnecessary leading newline
(whitespace)
[high] 760-760: G115: integer overflow conversion int -> uint64
(gosec)
[warning] 751-751: var-naming: don't use underscores in Go names; var gasLimit_fromEstimate should be gasLimitFromEstimate
(revive)
services/rfq/relayer/pricer/fee_pricer.go
266-266: unnecessary leading newline
(whitespace)
🪛 GitHub Check: Lint (services/rfq)
services/rfq/contracts/fastbridgev2/parser.go
[failure] 26-26:
Comment should end in a period (godot)
services/rfq/relayer/pricer/fee_pricer.go
[failure] 266-266:
unnecessary leading newline (whitespace)
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 428-428: ethergo/submitter/submitter.go#L428
Added line #L428 was not covered by tests
[warning] 431-431: ethergo/submitter/submitter.go#L431
Added line #L431 was not covered by tests
[warning] 435-435: ethergo/submitter/submitter.go#L435
Added line #L435 was not covered by tests
[warning] 440-440: ethergo/submitter/submitter.go#L440
Added line #L440 was not covered by tests
[warning] 442-442: ethergo/submitter/submitter.go#L442
Added line #L442 was not covered by tests
[warning] 447-447: ethergo/submitter/submitter.go#L447
Added line #L447 was not covered by tests
[warning] 459-459: ethergo/submitter/submitter.go#L459
Added line #L459 was not covered by tests
[warning] 461-461: ethergo/submitter/submitter.go#L461
Added line #L461 was not covered by tests
[warning] 471-471: ethergo/submitter/submitter.go#L471
Added line #L471 was not covered by tests
[warning] 726-727: ethergo/submitter/submitter.go#L726-L727
Added lines #L726 - L727 were not covered by tests
[warning] 732-733: ethergo/submitter/submitter.go#L732-L733
Added lines #L732 - L733 were not covered by tests
[warning] 737-737: ethergo/submitter/submitter.go#L737
Added line #L737 was not covered by tests
[warning] 751-752: ethergo/submitter/submitter.go#L751-L752
Added lines #L751 - L752 were not covered by tests
🪛 GitHub Check: Lint (ethergo)
ethergo/submitter/submitter.go
[failure] 425-425:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 465-465:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 466-466:
G115: integer overflow conversion int -> uint64 (gosec)
[failure] 426-426:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 430-430:
var-naming: don't use underscores in Go names; var transactor_forGasEstimate should be transactorForGasEstimate (revive)
[failure] 452-452:
var-naming: don't use underscores in Go names; var tx_forGasEstimate should be txForGasEstimate (revive)
[failure] 741-741:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 760-760:
G115: integer overflow conversion int -> uint64 (gosec)
[failure] 751-751:
var-naming: don't use underscores in Go names; var gasLimit_fromEstimate should be gasLimitFromEstimate (revive)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (ethergo)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Deploy to Vercel (docs)
🔇 Additional comments (6)
services/rfq/contracts/fastbridgev2/eventtype_string.go (1)
16-16: LGTM! Auto-generated code changes look correct.The stringer-generated code has been properly updated to include the new
BridgeQuoteDetailsEventtype.Also applies to: 19-19, 21-21
services/rfq/contracts/fastbridgev2/events.go (1)
72-72: LGTM! Topic mapping is correctly added.The new event type is properly mapped to its topic in the
topicMapfunction.services/rfq/contracts/fastbridgev2/parser.go (1)
96-101: LGTM! Event parsing implementation looks good.The new event type handling follows the established pattern and includes proper error handling.
services/rfq/relayer/pricer/fee_pricer.go (1)
264-274: LGTM! Good error handling.The error handling logic is well-implemented:
- Properly handles gas estimation errors
- Truncates HTML portion of error messages for better readability
- Returns a clear error message
🧰 Tools
🪛 golangci-lint (1.62.2)
266-266: unnecessary leading newline
(whitespace)
🪛 GitHub Check: Lint (services/rfq)
[failure] 266-266:
unnecessary leading newline (whitespace)ethergo/submitter/submitter.go (2)
Line range hint
725-764: LGTM! Good gas estimation logic.The gas estimation logic is well-implemented:
- Properly handles both dynamic and static gas estimation
- Adds configurable percentage to estimated gas
- Falls back to default gas limit on estimation failure
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 741-741: G115: integer overflow conversion uint64 -> int64
(gosec)
725-725: unnecessary leading newline
(whitespace)
🪛 GitHub Check: codecov/patch
[warning] 726-727: ethergo/submitter/submitter.go#L726-L727
Added lines #L726 - L727 were not covered by tests
[warning] 732-733: ethergo/submitter/submitter.go#L732-L733
Added lines #L732 - L733 were not covered by tests
[warning] 737-737: ethergo/submitter/submitter.go#L737
Added line #L737 was not covered by tests🪛 GitHub Check: Lint (ethergo)
[failure] 741-741:
G115: integer overflow conversion uint64 -> int64 (gosec)
422-468: Add test coverage for gas estimation logic.The gas estimation logic lacks test coverage, particularly for:
- Dynamic gas estimation
- Gas limit percentage adjustment
- Error handling
Run the following script to verify test coverage:
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 425-425: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 465-465: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 466-466: G115: integer overflow conversion int -> uint64
(gosec)
[high] 426-426: G115: integer overflow conversion uint64 -> int
(gosec)
[warning] 430-430: var-naming: don't use underscores in Go names; var transactor_forGasEstimate should be transactorForGasEstimate
(revive)
[warning] 452-452: var-naming: don't use underscores in Go names; var tx_forGasEstimate should be txForGasEstimate
(revive)
427-427: unnecessary leading newline
(whitespace)
434-434: unnecessary leading newline
(whitespace)
🪛 GitHub Check: codecov/patch
[warning] 428-428: ethergo/submitter/submitter.go#L428
Added line #L428 was not covered by tests
[warning] 431-431: ethergo/submitter/submitter.go#L431
Added line #L431 was not covered by tests
[warning] 435-435: ethergo/submitter/submitter.go#L435
Added line #L435 was not covered by tests
[warning] 440-440: ethergo/submitter/submitter.go#L440
Added line #L440 was not covered by tests
[warning] 442-442: ethergo/submitter/submitter.go#L442
Added line #L442 was not covered by tests
[warning] 447-447: ethergo/submitter/submitter.go#L447
Added line #L447 was not covered by tests
[warning] 459-459: ethergo/submitter/submitter.go#L459
Added line #L459 was not covered by tests
[warning] 461-461: ethergo/submitter/submitter.go#L461
Added line #L461 was not covered by tests🪛 GitHub Check: Lint (ethergo)
[failure] 425-425:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 465-465:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 466-466:
G115: integer overflow conversion int -> uint64 (gosec)
[failure] 426-426:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 430-430:
var-naming: don't use underscores in Go names; var transactor_forGasEstimate should be transactorForGasEstimate (revive)
[failure] 452-452:
var-naming: don't use underscores in Go names; var tx_forGasEstimate should be txForGasEstimate (revive)
| @@ -64,6 +67,9 @@ const ( | |||
|
|
|||
| // 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 | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
ethergo/submitter/submitter.go
Outdated
| } else { | ||
|
|
||
| // deepcopy the real transactor so we can use it for simulation | ||
| transactor_forGasEstimate := copyTransactOpts(transactor) |
There was a problem hiding this comment.
Small nit but may be better to avoid underscores in var names for style purposes
There was a problem hiding this comment.
NP - will remove & stay consistent
| @@ -86,10 +86,19 @@ func (c Chain) SubmitRelay(ctx context.Context, request reldb.QuoteRequest) (uin | |||
| } | |||
| } | |||
|
|
|||
| fmt.Printf( | |||
There was a problem hiding this comment.
Should we trace this instead?
There was a problem hiding this comment.
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
|
|
||
| // at the moment, omniRPC gives a massive HTML doc w/ many sim errors.. reduce the noise. | ||
| errMsg := err.Error() | ||
| if strings.Contains(errMsg, "<!DOCTYPE html>") { |
There was a problem hiding this comment.
I think we could make a shared util for this so that we don't duplicate, something like util.StripHTML(errMsg)
There was a problem hiding this comment.
yeah thats a good thought - will try that
| @@ -470,11 +471,19 @@ func (q *QuoteRequestHandler) handleRelayCompleted(ctx context.Context, span tra | |||
| return nil | |||
| } | |||
|
|
|||
| fmt.Printf( | |||
ethergo/submitter/submitter.go
Outdated
|
|
||
| // 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 | ||
| transactor_forGasEstimate.Signer = func(address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) { |
There was a problem hiding this comment.
This is purely for deduplication and readability, but may be worth trying- it seems that the two Signer funcs are almost identical, but one locks the mux and one does not
Perhaps we could define the common signing logic under one inline function, like this:
...
performSignature := func (address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) {
...
}
...
And then the part that performs the locking would just look like this:
...
transactor.Signer = func (address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) {
locker = t.nonceMux.Lock(chainID)
// it's important that we unlock the nonce if we fail to sign the transaction.
// this is why we use a defer here. The second defer should only be called if the first defer is not called.
defer func() {
if err != nil {
locker.Unlock()
}
}()
performSignature(address, transaction)
}
...
Does that make sense?
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
ethergo/util/util.go (1)
13-15: Consider using strings.SplitN for better performance.Since we only need the first part before the HTML, using
strings.SplitNwould be more efficient thanstrings.Split.- errMsg = strings.Split(errMsg, "<!DOCTYPE html>")[0] + "<html portion of error removed>" + errMsg = strings.SplitN(errMsg, "<!DOCTYPE html>", 2)[0] + "<html portion of error removed>"services/rfq/relayer/pricer/fee_pricer.go (1)
267-271: Remove unnecessary newlines in error handling.The error handling block contains unnecessary newlines that affect readability.
gasEstimate, err = client.EstimateGas(ctx, callMsg) if err != nil { - errMsg := ethergoUtil.FormatError(err) - return 0, fmt.Errorf("could not estimate gas: %s", errMsg) }🧰 Tools
🪛 golangci-lint (1.62.2)
267-267: unnecessary leading newline
(whitespace)
🪛 GitHub Check: Lint (services/rfq)
[failure] 267-267:
unnecessary leading newline (whitespace)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)ethergo/submitter/submitter.go(3 hunks)ethergo/util/util.go(1 hunks)services/rfq/relayer/pricer/fee_pricer.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ethergo/util/util.go
12-12: commentFormatting: put a space between // and comment text
(gocritic)
ethergo/submitter/submitter.go
[high] 424-424: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 460-460: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 461-461: G115: integer overflow conversion int -> uint64
(gosec)
[high] 425-425: G115: integer overflow conversion uint64 -> int
(gosec)
426-426: unnecessary leading newline
(whitespace)
433-433: unnecessary leading newline
(whitespace)
[high] 736-736: G115: integer overflow conversion uint64 -> int64
(gosec)
720-720: unnecessary leading newline
(whitespace)
[high] 756-756: G115: integer overflow conversion int -> uint64
(gosec)
services/rfq/relayer/pricer/fee_pricer.go
267-267: unnecessary leading newline
(whitespace)
🪛 GitHub Check: Lint (ethergo)
ethergo/util/util.go
[failure] 12-12:
commentFormatting: put a space between // and comment text (gocritic)
ethergo/submitter/submitter.go
[failure] 424-424:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 460-460:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 461-461:
G115: integer overflow conversion int -> uint64 (gosec)
[failure] 425-425:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 426-426:
unnecessary leading newline (whitespace)
[failure] 433-433:
unnecessary leading newline (whitespace)
[failure] 736-736:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 756-756:
G115: integer overflow conversion int -> uint64 (gosec)
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 427-428: ethergo/submitter/submitter.go#L427-L428
Added lines #L427 - L428 were not covered by tests
[warning] 430-432: ethergo/submitter/submitter.go#L430-L432
Added lines #L430 - L432 were not covered by tests
[warning] 434-438: ethergo/submitter/submitter.go#L434-L438
Added lines #L434 - L438 were not covered by tests
[warning] 440-445: ethergo/submitter/submitter.go#L440-L445
Added lines #L440 - L445 were not covered by tests
[warning] 448-448: ethergo/submitter/submitter.go#L448
Added line #L448 was not covered by tests
[warning] 452-452: ethergo/submitter/submitter.go#L452
Added line #L452 was not covered by tests
[warning] 455-455: ethergo/submitter/submitter.go#L455
Added line #L455 was not covered by tests
[warning] 460-460: ethergo/submitter/submitter.go#L460
Added line #L460 was not covered by tests
[warning] 466-466: ethergo/submitter/submitter.go#L466
Added line #L466 was not covered by tests
[warning] 720-722: ethergo/submitter/submitter.go#L720-L722
Added lines #L720 - L722 were not covered by tests
[warning] 727-728: ethergo/submitter/submitter.go#L727-L728
Added lines #L727 - L728 were not covered by tests
[warning] 732-732: ethergo/submitter/submitter.go#L732
Added line #L732 was not covered by tests
[warning] 736-736: ethergo/submitter/submitter.go#L736
Added line #L736 was not covered by tests
[warning] 747-747: ethergo/submitter/submitter.go#L747
Added line #L747 was not covered by tests
[warning] 751-751: ethergo/submitter/submitter.go#L751
Added line #L751 was not covered by tests
[warning] 758-759: ethergo/submitter/submitter.go#L758-L759
Added lines #L758 - L759 were not covered by tests
🪛 GitHub Check: Lint (services/rfq)
services/rfq/relayer/pricer/fee_pricer.go
[failure] 267-267:
unnecessary leading newline (whitespace)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Go Generate (Module Changes) (tools)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/cctp-relayer)
- GitHub Check: Go Generate (Module Changes) (ethergo)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (contrib/opbot)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Go Coverage (1.22.x, ethergo)
- GitHub Check: Deploy to Vercel (docs)
🔇 Additional comments (3)
services/rfq/relayer/pricer/fee_pricer.go (2)
259-264: LGTM! Good use of utility function for gas token check.The code correctly uses
rfqUtil.IsGasTokento determine the appropriate value for the call message.
269-269: Good integration of error formatting.The use of
ethergoUtil.FormatErroraligns with the past review comment suggesting a shared utility for error message formatting.ethergo/submitter/submitter.go (1)
427-462: Add test coverage for dynamic gas estimation.The dynamic gas estimation logic lacks test coverage, particularly for:
- Gas estimation simulation
- Gas limit percentage adjustment
- Error handling paths
✅ Verification successful
Add test coverage for dynamic gas estimation logic
The codebase search confirms that test coverage is missing for the dynamic gas estimation implementation in
ethergo/submitter/submitter.go. Critical paths that need test coverage:
- Dynamic gas estimation simulation
- Gas limit percentage adjustment calculation
- Error handling for simulation failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test coverage for dynamic gas estimation echo "=== Checking test coverage ===" go test -coverprofile=coverage.out ./ethergo/submitter/ go tool cover -func=coverage.out | grep "getGasEstimate" echo -e "\n=== Searching for relevant test files ===" rg -g "*_test.go" "GetDynamicGasEstimate|gasUnitAddPercentage" -A 5Length of output: 12996
Script:
#!/bin/bash # Search for gas estimation related code and tests echo "=== Searching for gas estimation implementation ===" rg -g "!*_test.go" "func.*[Gg]as[Ee]stimate|gasUnitAddPercentage" -A 5 echo -e "\n=== Searching for gas estimation tests with different patterns ===" rg -g "*_test.go" "Test.*[Gg]as|gasLimit|gasUnitAddPercentage" -A 5 echo -e "\n=== Looking for mock implementations ===" rg -g "*_mock.go" "[Gg]as[Ee]stimate|gasLimit|gasUnitAddPercentage"Length of output: 34109
🧰 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)
433-433: unnecessary leading newline
(whitespace)
🪛 GitHub Check: codecov/patch
[warning] 427-428: ethergo/submitter/submitter.go#L427-L428
Added lines #L427 - L428 were not covered by tests
[warning] 430-432: ethergo/submitter/submitter.go#L430-L432
Added lines #L430 - L432 were not covered by tests
[warning] 434-438: ethergo/submitter/submitter.go#L434-L438
Added lines #L434 - L438 were not covered by tests
[warning] 440-445: ethergo/submitter/submitter.go#L440-L445
Added lines #L440 - L445 were not covered by tests
[warning] 448-448: ethergo/submitter/submitter.go#L448
Added line #L448 was not covered by tests
[warning] 452-452: ethergo/submitter/submitter.go#L452
Added line #L452 was not covered by tests
[warning] 455-455: ethergo/submitter/submitter.go#L455
Added line #L455 was not covered by tests
[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)
[failure] 433-433:
unnecessary leading newline (whitespace)
| } | ||
| errMsg := err.Error() | ||
|
|
||
| //if an error message contains embedded HTML (eg: many RPC errors), strip it out to reduce noise. |
There was a problem hiding this comment.
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.
| //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)
| // multiply the freshly simulated gasLimit by the configured gas unit add percentage | ||
| gasLimitFromEstimate += (gasLimitFromEstimate * uint64(gasUnitAddPercentage) / 100) | ||
| gasLimit = gasLimitFromEstimate |
There was a problem hiding this comment.
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.
| // 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)
| if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) { | ||
| transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) | ||
| } else { |
There was a problem hiding this comment.
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.
| 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)
| gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainID.Uint64())) | ||
| transactor.GasLimit = txForGasEstimate.Gas() + (txForGasEstimate.Gas() * uint64(gasLimitAddPercentage) / 100) |
There was a problem hiding this comment.
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.
| 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)
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
ethergo/submitter/submitter.go (1)
Line range hint
711-750: Protect against integer overflow in gas calculations.The gas limit percentage calculation could overflow when dealing with large gas values.
Apply this diff to fix the overflow risks:
- 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()🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 727-727: G115: integer overflow conversion uint64 -> int64
(gosec)
711-711: unnecessary leading newline
(whitespace)
🪛 GitHub Check: codecov/patch
[warning] 711-713: ethergo/submitter/submitter.go#L711-L713
Added lines #L711 - L713 were not covered by tests
[warning] 718-719: ethergo/submitter/submitter.go#L718-L719
Added lines #L718 - L719 were not covered by tests
[warning] 723-723: ethergo/submitter/submitter.go#L723
Added line #L723 was not covered by tests
[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)
🧹 Nitpick comments (1)
ethergo/submitter/submitter.go (1)
396-411: Good refactor, but protect against integer overflow.The extraction of common signing logic into
performSignatureimproves code organization. However, there's a potential integer overflow intxTypeForChainwhen convertingchainID.Uint64()to int.Add overflow protection:
func (t *txSubmitterImpl) txTypeForChain(chainID *big.Int) (txType uint8) { + chainIDInt := chainID.Int64() + if chainIDInt > math.MaxInt32 { + return types.LegacyTxType // fallback to legacy type for very large chain IDs + } - if t.config.SupportsEIP1559(int(chainID.Uint64())) { + if t.config.SupportsEIP1559(int(chainIDInt)) { txType = types.DynamicFeeTxType } else { txType = types.LegacyTxType } return txType }🧰 Tools
🪛 golangci-lint (1.62.2)
396-396: unnecessary leading newline
(whitespace)
🪛 GitHub Check: codecov/patch
[warning] 400-401: ethergo/submitter/submitter.go#L400-L401
Added lines #L400 - L401 were not covered by tests
[warning] 407-408: ethergo/submitter/submitter.go#L407-L408
Added lines #L407 - L408 were not covered by tests🪛 GitHub Check: Lint (ethergo)
[failure] 396-396:
unnecessary leading newline (whitespace)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ethergo/submitter/submitter.go(4 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ethergo/submitter/submitter.go
396-396: unnecessary leading newline
(whitespace)
[high] 429-429: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 451-451: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 452-452: G115: integer overflow conversion int -> uint64
(gosec)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
431-431: unnecessary leading newline
(whitespace)
[high] 727-727: G115: integer overflow conversion uint64 -> int64
(gosec)
711-711: unnecessary leading newline
(whitespace)
[high] 747-747: G115: integer overflow conversion int -> uint64
(gosec)
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 400-401: ethergo/submitter/submitter.go#L400-L401
Added lines #L400 - L401 were not covered by tests
[warning] 407-408: ethergo/submitter/submitter.go#L407-L408
Added lines #L407 - L408 were not covered by tests
[warning] 433-438: ethergo/submitter/submitter.go#L433-L438
Added lines #L433 - L438 were not covered by tests
[warning] 442-442: ethergo/submitter/submitter.go#L442
Added line #L442 was not covered by tests
[warning] 444-446: ethergo/submitter/submitter.go#L444-L446
Added lines #L444 - L446 were not covered by tests
[warning] 451-452: ethergo/submitter/submitter.go#L451-L452
Added lines #L451 - L452 were not covered by tests
[warning] 457-457: ethergo/submitter/submitter.go#L457
Added line #L457 was not covered by tests
[warning] 711-713: ethergo/submitter/submitter.go#L711-L713
Added lines #L711 - L713 were not covered by tests
[warning] 718-719: ethergo/submitter/submitter.go#L718-L719
Added lines #L718 - L719 were not covered by tests
[warning] 723-723: ethergo/submitter/submitter.go#L723
Added line #L723 was not covered by tests
[warning] 727-727: ethergo/submitter/submitter.go#L727
Added line #L727 was not covered by tests
[warning] 737-738: ethergo/submitter/submitter.go#L737-L738
Added lines #L737 - L738 were not covered by tests
[warning] 742-742: ethergo/submitter/submitter.go#L742
Added line #L742 was not covered by tests
[warning] 747-750: ethergo/submitter/submitter.go#L747-L750
Added lines #L747 - L750 were not covered by tests
🪛 GitHub Check: Lint (ethergo)
ethergo/submitter/submitter.go
[failure] 396-396:
unnecessary leading newline (whitespace)
[failure] 429-429:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 451-451:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 452-452:
G115: integer overflow conversion int -> uint64 (gosec)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 431-431:
unnecessary leading newline (whitespace)
[failure] 727-727:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 747-747:
G115: integer overflow conversion int -> uint64 (gosec)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Go Generate (Module Changes) (tools)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Go Coverage (1.22.x, ethergo)
- GitHub Check: Deploy to Vercel (docs)
- GitHub Check: Cloudflare Pages
| )) | ||
|
|
||
| defer func() { | ||
| span.AddEvent("estimated_gas", trace.WithAttributes(attribute.Int64("gas", int64(gasEstimate)))) | ||
| span.AddEvent("estimated_gas", trace.WithAttributes(attribute.Int64("gas", int64(gasLimit)))) |
There was a problem hiding this comment.
🛠️ 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.
| 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)
| // 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())) { | ||
| transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) | ||
| } else { | ||
|
|
||
| transaction, err = util.CopyTX(transaction, util.WithNonce(newNonce), util.WithTxType(txType)) | ||
| // deepcopy the real transactor so we can use it for simulation | ||
| 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 | ||
| transactorForGasEstimate.Signer = func(address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) { | ||
| return performSignature(address, transaction) | ||
| } | ||
|
|
||
| txForGasEstimate, err := call(transactorForGasEstimate) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not copy tx: %w", err) | ||
| errMsg := util.FormatError(err) | ||
|
|
||
| return 0, fmt.Errorf("err contract call for gas est: %s", errMsg) | ||
| } | ||
|
|
||
| //nolint: wrapcheck | ||
| return parentTransactor.Signer(address, transaction) | ||
| // 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())) | ||
| transactor.GasLimit = txForGasEstimate.Gas() + (txForGasEstimate.Gas() * uint64(gasLimitAddPercentage) / 100) |
There was a problem hiding this comment.
Protect against multiple integer overflows.
The code has several potential integer overflow issues:
- Converting
chainID.Uint64()to int - Converting gas limit percentage to uint64
- Gas limit multiplication and addition
Apply this diff to fix the overflow risks:
+ chainIDInt := chainID.Int64()
+ if chainIDInt > math.MaxInt32 {
+ return 0, fmt.Errorf("chain ID %d exceeds maximum supported value", chainIDInt)
+ }
- if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) {
- transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64()))
+ if !t.config.GetDynamicGasEstimate(int(chainIDInt)) {
+ transactor.GasLimit = t.config.GetGasEstimate(int(chainIDInt))
} else {
- gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainID.Uint64()))
- transactor.GasLimit = txForGasEstimate.Gas() + (txForGasEstimate.Gas() * uint64(gasLimitAddPercentage) / 100)
+ gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainIDInt))
+ 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.
| // 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())) { | |
| transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) | |
| } else { | |
| transaction, err = util.CopyTX(transaction, util.WithNonce(newNonce), util.WithTxType(txType)) | |
| // deepcopy the real transactor so we can use it for simulation | |
| 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 | |
| transactorForGasEstimate.Signer = func(address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) { | |
| return performSignature(address, transaction) | |
| } | |
| txForGasEstimate, err := call(transactorForGasEstimate) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not copy tx: %w", err) | |
| errMsg := util.FormatError(err) | |
| return 0, fmt.Errorf("err contract call for gas est: %s", errMsg) | |
| } | |
| //nolint: wrapcheck | |
| return parentTransactor.Signer(address, transaction) | |
| // 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())) | |
| transactor.GasLimit = txForGasEstimate.Gas() + (txForGasEstimate.Gas() * uint64(gasLimitAddPercentage) / 100) | |
| // 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 | |
| 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 { | |
| // deepcopy the real transactor so we can use it for simulation | |
| 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 | |
| transactorForGasEstimate.Signer = func(address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) { | |
| return performSignature(address, transaction) | |
| } | |
| txForGasEstimate, err := call(transactorForGasEstimate) | |
| if err != nil { | |
| errMsg := util.FormatError(err) | |
| return 0, fmt.Errorf("err contract call for gas est: %s", errMsg) | |
| } | |
| // 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(chainIDInt)) | |
| 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] 429-429: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 451-451: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 452-452: G115: integer overflow conversion int -> uint64
(gosec)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
431-431: unnecessary leading newline
(whitespace)
🪛 GitHub Check: codecov/patch
[warning] 433-438: ethergo/submitter/submitter.go#L433-L438
Added lines #L433 - L438 were not covered by tests
[warning] 442-442: ethergo/submitter/submitter.go#L442
Added line #L442 was not covered by tests
[warning] 444-446: ethergo/submitter/submitter.go#L444-L446
Added lines #L444 - L446 were not covered by tests
[warning] 451-452: ethergo/submitter/submitter.go#L451-L452
Added lines #L451 - L452 were not covered by tests
🪛 GitHub Check: Lint (ethergo)
[failure] 429-429:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 451-451:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 452-452:
G115: integer overflow conversion int -> uint64 (gosec)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 431-431:
unnecessary leading newline (whitespace)
💡 Codebase verification
Test coverage for dynamic gas estimation is indeed missing
The codebase lacks proper test coverage for the dynamic gas estimation functionality. Critical areas requiring test coverage:
- Dynamic gas estimation logic in the submitter package
- Gas limit percentage adjustments
- Error handling for estimation failures
- Integration tests with different chain configurations
🔗 Analysis chain
Add test coverage for dynamic gas estimation.
The new gas estimation logic lacks test coverage, particularly for:
- Dynamic gas estimation configuration
- Gas limit percentage adjustments
- Error handling for estimation failures
Run this script to verify test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for gas estimation logic
echo "=== Checking test coverage ==="
go test -coverprofile=coverage.out ./ethergo/submitter/
go tool cover -func=coverage.out | grep "getGasEstimate"
echo -e "\n=== Searching for gas estimation tests ==="
rg -l "TestGetGasEstimate|TestDynamicGasEstimate" --type go --glob "*_test.go"
Length of output: 13718
Script:
#!/bin/bash
# Search for test files and their content related to gas estimation
echo "=== Searching for gas estimation related test files ==="
rg -l "gas.*estimate" --type go --glob "*_test.go"
echo -e "\n=== Searching for gas estimation test cases ==="
rg "func.*[Tt]est.*[Gg]as.*[Ee]stimate" --type go --glob "*_test.go" -A 5
echo -e "\n=== Searching for dynamic gas configuration tests ==="
rg "DynamicGasEstimate|GetDynamicGasEstimate" --type go --glob "*_test.go" -A 5
Length of output: 868
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 429-429: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 451-451: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 452-452: G115: integer overflow conversion int -> uint64
(gosec)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
431-431: unnecessary leading newline
(whitespace)
🪛 GitHub Check: codecov/patch
[warning] 433-438: ethergo/submitter/submitter.go#L433-L438
Added lines #L433 - L438 were not covered by tests
[warning] 442-442: ethergo/submitter/submitter.go#L442
Added line #L442 was not covered by tests
[warning] 444-446: ethergo/submitter/submitter.go#L444-L446
Added lines #L444 - L446 were not covered by tests
[warning] 451-452: ethergo/submitter/submitter.go#L451-L452
Added lines #L451 - L452 were not covered by tests
🪛 GitHub Check: Lint (ethergo)
[failure] 429-429:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 451-451:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 452-452:
G115: integer overflow conversion int -> uint64 (gosec)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 431-431:
unnecessary leading newline (whitespace)
allows gas units/limit to be increased by X% beyond the value returned by estimateGas
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores
.gitignoreto exclude specific configuration and signer files.Documentation
95798a8: docs preview link
995e2b7: docs preview link
1f734dd: docs preview link
d01551c: docs preview link
fda62d9: docs preview link
d5e3394: docs preview link
2020bcd: docs preview link
e55e7c1: docs preview link
7704c18: docs preview link
9a2579e: docs preview link
78e9503: docs preview link
09bb89f: docs preview link
fe184df: docs preview link
8fcf948: docs preview link
af569f7: docs preview link
cd68b39: docs preview link
0e9cd9c: docs preview link
1280b83: docs preview link
c22a87c: docs preview link
087a545: docs preview link
8b3507e: docs preview link
6c1586b: docs preview link
cdcd69e: docs preview link
9af9a2c: docs preview link
96af4f6: docs preview link
e93dce8: docs preview link
6f21782: docs preview link
c22edab: docs preview link
a2a905e: docs preview link
ecdefa9: docs preview link
fc66c45: docs preview link
7a472dd: docs preview link
03ec0e4: docs preview link
c841fb4: docs preview link
3b3102c: docs preview link
5e89497: docs preview link
b1a3ca6: docs preview link
c6f6488: docs preview link
7413d3c: docs preview link
088d251: docs preview link