Submitter/dynamic tx fees [goreleaser] #2432
Conversation
WalkthroughThis update introduces several enhancements and adjustments across the transaction processing and metric tracking systems. Key modifications include dynamic fee adjustments for transactions, a new environment variable for metric synchronization, improved error logging consistency, and refined logic for transaction status updates. These changes aim to optimize transaction fee management, enhance monitoring capabilities, and streamline transaction processing workflows. Changes
Possibly related issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Deploying sanguine-fe with
|
| Latest commit: |
ba5f437
|
| Status: | 🚫 Build failed. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2432 +/- ##
===================================================
+ Coverage 46.91894% 47.06629% +0.14735%
===================================================
Files 400 400
Lines 30655 30729 +74
Branches 83 83
===================================================
+ Hits 14383 14463 +80
+ Misses 14737 14729 -8
- Partials 1535 1537 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- ethergo/chain/gas/cmp.go (1 hunks)
Additional comments not posted (1)
ethergo/chain/gas/cmp.go (1)
88-113: The implementation ofbumpDynamicTxFeesandmaxBumpedFeefunctions demonstrates good modularity and maintainability.
ethergo/chain/gas/cmp.go
Outdated
| // Calculate new tip cap as a percentage increase over the current tip cap | ||
| newTipCap := BumpByPercent(opts.GasTipCap, percentIncrease) | ||
|
|
||
| // if new fee cap less than tip cap AND base (fee + fee cap) > tip cap | ||
| if maxPrice.Cmp(newTipCap) > 0 && big.NewInt(0).Sub(maxPrice, baseFee).Cmp(newTipCap) > 0 { | ||
| opts.GasTipCap = newTipCap | ||
| logger.Warnf("new tip cap %s still less than fee cap %s + base fee %s, bumping tip not and not fee", newTipCap, maxPrice, baseFee) | ||
| // Update bumpedTipCap if currentTipCap is higher than bumpedTipCap and within maxGasPrice | ||
| newTipCap = maxBumpedFee(opts.GasTipCap, newTipCap, maxPrice, "tip cap") | ||
|
|
||
| // Adjust the new tip cap if it exceeds max price | ||
| if newTipCap.Cmp(maxPrice) > 0 { | ||
| newTipCap = maxPrice | ||
| logger.Warnf("Adjusted new tip cap %s exceeds max price %s, using max price as tip cap", newTipCap, maxPrice) | ||
| } | ||
|
|
||
| // Calculate new fee cap as a percentage increase over the current fee cap | ||
| newFeeCap := BumpByPercent(opts.GasFeeCap, percentIncrease) | ||
| if maxPrice.Cmp(newFeeCap) > 0 { | ||
| opts.GasFeeCap = newFeeCap | ||
| } else { | ||
| opts.GasFeeCap = maxPrice | ||
| logger.Warnf("new fee cap %s exceeds max price %s, using max price", newFeeCap, maxPrice) | ||
|
|
||
| // Ensure the bumped fee cap does not exceed the max gas price | ||
| if newFeeCap.Cmp(maxPrice) > 0 { | ||
| logger.Errorf("bumped fee cap of %s would exceed configured max gas price of %s (original fee: tip cap %s, fee cap %s).", | ||
| newFeeCap.String(), maxPrice, opts.GasTipCap.String(), opts.GasFeeCap.String()) | ||
| newFeeCap = maxPrice | ||
| } | ||
|
|
||
| // Apply the calculated tip cap and fee cap to the transaction options | ||
| opts.GasTipCap = newTipCap | ||
| opts.GasFeeCap = newFeeCap | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for the case when percentIncrease is negative.
func bumpDynamicTxFees(opts *bind.TransactOpts, percentIncrease int, baseFee, maxPrice *big.Int) {
+ if percentIncrease < 0 {
+ logger.Errorf("percentIncrease cannot be negative: %d", percentIncrease)
+ return
+ }
// Calculate new tip cap as a percentage increase over the current tip cap
newTipCap := BumpByPercent(opts.GasTipCap, percentIncrease)
...
}This ensures that the function gracefully handles invalid input values, improving the robustness of the fee adjustment logic.
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.
| // Calculate new tip cap as a percentage increase over the current tip cap | |
| newTipCap := BumpByPercent(opts.GasTipCap, percentIncrease) | |
| // if new fee cap less than tip cap AND base (fee + fee cap) > tip cap | |
| if maxPrice.Cmp(newTipCap) > 0 && big.NewInt(0).Sub(maxPrice, baseFee).Cmp(newTipCap) > 0 { | |
| opts.GasTipCap = newTipCap | |
| logger.Warnf("new tip cap %s still less than fee cap %s + base fee %s, bumping tip not and not fee", newTipCap, maxPrice, baseFee) | |
| // Update bumpedTipCap if currentTipCap is higher than bumpedTipCap and within maxGasPrice | |
| newTipCap = maxBumpedFee(opts.GasTipCap, newTipCap, maxPrice, "tip cap") | |
| // Adjust the new tip cap if it exceeds max price | |
| if newTipCap.Cmp(maxPrice) > 0 { | |
| newTipCap = maxPrice | |
| logger.Warnf("Adjusted new tip cap %s exceeds max price %s, using max price as tip cap", newTipCap, maxPrice) | |
| } | |
| // Calculate new fee cap as a percentage increase over the current fee cap | |
| newFeeCap := BumpByPercent(opts.GasFeeCap, percentIncrease) | |
| if maxPrice.Cmp(newFeeCap) > 0 { | |
| opts.GasFeeCap = newFeeCap | |
| } else { | |
| opts.GasFeeCap = maxPrice | |
| logger.Warnf("new fee cap %s exceeds max price %s, using max price", newFeeCap, maxPrice) | |
| // Ensure the bumped fee cap does not exceed the max gas price | |
| if newFeeCap.Cmp(maxPrice) > 0 { | |
| logger.Errorf("bumped fee cap of %s would exceed configured max gas price of %s (original fee: tip cap %s, fee cap %s).", | |
| newFeeCap.String(), maxPrice, opts.GasTipCap.String(), opts.GasFeeCap.String()) | |
| newFeeCap = maxPrice | |
| } | |
| // Apply the calculated tip cap and fee cap to the transaction options | |
| opts.GasTipCap = newTipCap | |
| opts.GasFeeCap = newFeeCap | |
| } | |
| func bumpDynamicTxFees(opts *bind.TransactOpts, percentIncrease int, baseFee, maxPrice *big.Int) { | |
| if percentIncrease < 0 { | |
| logger.Errorf("percentIncrease cannot be negative: %d", percentIncrease) | |
| return | |
| } | |
| // Calculate new tip cap as a percentage increase over the current tip cap | |
| newTipCap := BumpByPercent(opts.GasTipCap, percentIncrease) | |
| // Update bumpedTipCap if currentTipCap is higher than bumpedTipCap and within maxGasPrice | |
| newTipCap = maxBumpedFee(opts.GasTipCap, newTipCap, maxPrice, "tip cap") | |
| // Adjust the new tip cap if it exceeds max price | |
| if newTipCap.Cmp(maxPrice) > 0 { | |
| newTipCap = maxPrice | |
| logger.Warnf("Adjusted new tip cap %s exceeds max price %s, using max price as tip cap", newTipCap, maxPrice) | |
| } | |
| // Calculate new fee cap as a percentage increase over the current fee cap | |
| newFeeCap := BumpByPercent(opts.GasFeeCap, percentIncrease) | |
| // Ensure the bumped fee cap does not exceed the max gas price | |
| if newFeeCap.Cmp(maxPrice) > 0 { | |
| logger.Errorf("bumped fee cap of %s would exceed configured max gas price of %s (original fee: tip cap %s, fee cap %s).", | |
| newFeeCap.String(), maxPrice, opts.GasTipCap.String(), opts.GasFeeCap.String()) | |
| newFeeCap = maxPrice | |
| } | |
| // Apply the calculated tip cap and fee cap to the transaction options | |
| opts.GasTipCap = newTipCap | |
| opts.GasFeeCap = newFeeCap | |
| } |
ethergo/chain/gas/cmp.go
Outdated
| func maxBumpedFee(currentFeePrice, bumpedFeePrice, maxGasPrice *big.Int, feeType string) *big.Int { | ||
| if currentFeePrice != nil { | ||
| if currentFeePrice.Cmp(maxGasPrice) > 0 { | ||
| // Shouldn't happen because the estimator should not be allowed to | ||
| // estimate a higher gas than the maximum allowed | ||
| logger.Warnf("Ignoring current %s of %s that would exceed max %s of %s", feeType, currentFeePrice.String(), feeType, maxGasPrice.String()) | ||
| } else if bumpedFeePrice.Cmp(currentFeePrice) < 0 { | ||
| // If the current gas price is higher than the old price bumped, use that instead | ||
| bumpedFeePrice = currentFeePrice | ||
| } | ||
| } | ||
| return bumpedFeePrice |
There was a problem hiding this comment.
Ensure consistent logging levels for similar conditions.
if currentFeePrice.Cmp(maxGasPrice) > 0 {
- logger.Warnf("Ignoring current %s of %s that would exceed max %s of %s", feeType, currentFeePrice.String(), feeType, maxGasPrice.String())
+ logger.Errorf("Ignoring current %s of %s that would exceed max %s of %s", feeType, currentFeePrice.String(), feeType, maxGasPrice.String())
}Using Errorf instead of Warnf aligns the logging level with the severity of exceeding the maximum gas price, enhancing the consistency and clarity of log messages.
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.
| func maxBumpedFee(currentFeePrice, bumpedFeePrice, maxGasPrice *big.Int, feeType string) *big.Int { | |
| if currentFeePrice != nil { | |
| if currentFeePrice.Cmp(maxGasPrice) > 0 { | |
| // Shouldn't happen because the estimator should not be allowed to | |
| // estimate a higher gas than the maximum allowed | |
| logger.Warnf("Ignoring current %s of %s that would exceed max %s of %s", feeType, currentFeePrice.String(), feeType, maxGasPrice.String()) | |
| } else if bumpedFeePrice.Cmp(currentFeePrice) < 0 { | |
| // If the current gas price is higher than the old price bumped, use that instead | |
| bumpedFeePrice = currentFeePrice | |
| } | |
| } | |
| return bumpedFeePrice | |
| func maxBumpedFee(currentFeePrice, bumpedFeePrice, maxGasPrice *big.Int, feeType string) *big.Int { | |
| if currentFeePrice != nil { | |
| if currentFeePrice.Cmp(maxGasPrice) > 0 { | |
| // Shouldn't happen because the estimator should not be allowed to | |
| // estimate a higher gas than the maximum allowed | |
| logger.Errorf("Ignoring current %s of %s that would exceed max %s of %s", feeType, currentFeePrice.String(), feeType, maxGasPrice.String()) | |
| } else if bumpedFeePrice.Cmp(currentFeePrice) < 0 { | |
| // If the current gas price is higher than the old price bumped, use that instead | |
| bumpedFeePrice = currentFeePrice | |
| } | |
| } | |
| return bumpedFeePrice | |
| } |
core/metrics/README.md
Outdated
|
|
||
| The metrics endpoint is exposed on `/metrics` on port `8080` by default and is compatible with prometheus. The following options control the metrics endpoint: | ||
|
|
||
| | Enviornment Variable | Description | Default | | ||
| |------------------------|-----------------------------------------------|------------| | ||
| | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` | | ||
| | `METRICS_PORT` | Port to serve metrics on. | `8080` | | ||
| | `METRICS_PATH` | Path to serve metrics on | `/metrics` | | ||
| | Enviornment Variable | Description | Default | | ||
| |------------------------|-----------------------------------------------------------------------------|------------| | ||
| | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` | | ||
| | `METRICS_PORT` | Port to serve metrics on. | `8080` | | ||
| | `METRICS_PATH` | Path to serve metrics on | `/metrics` | | ||
| | `METRICS_ALWAYS_SYNC` | Wether or not to wait until metrics have been exported before ending a span | `false` | | ||
|
|
||
| **Note: this server failing to bind to `METRICS_PORT` will not cause the application to fail to start. The error will be logged.** | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [3-3]
Add a space after "metrics." and correct the spelling of "application".
- The metrics package contains standard drivers for opentracing, profiling and metrics.You can enable this in your appplication by calling `metrics.Setup()`.
+ The metrics package contains standard drivers for opentracing, profiling and metrics. You can enable this in your application by calling `metrics.Setup()`.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [7-7]
Correct the spelling of "environment".
- | OTLP | [OTLP Exporter](https://opentelemetry.io/docs/specs/otel/protocol/exporter/) protocol. Supported by various external providers including [New Relic](https://docs.newrelic.com/docs/more-integrations/open-source-telemetry-integrations/opentelemetry/opentelemetry-introduction/), [Signoz](https://signoz.io/blog/opentelemetry-collector-complete-guide/), [Grafana](https://grafana.com/docs/opentelemetry/collector/) and more | ✅ | ✅ | ❌ (but it can through pyroscope, by specifying the `PYROSCOPE_ENDPOINT` enviornment variable)
+ | OTLP | [OTLP Exporter](https://opentelemetry.io/docs/specs/otel/protocol/exporter/) protocol. Supported by various external providers including [New Relic](https://docs.newrelic.com/docs/more-integrations/open-source-telemetry-integrations/opentelemetry/opentelemetry-introduction/), [Signoz](https://signoz.io/blog/opentelemetry-collector-complete-guide/), [Grafana](https://grafana.com/docs/opentelemetry/collector/) and more | ✅ | ✅ | ❌ (but it can through pyroscope, by specifying the `PYROSCOPE_ENDPOINT` environment variable)📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [43-43]
Correct the spelling of "need".
- Since this is in beta, you ned to call `handler.ExperimentalLogger`.
+ Since this is in beta, you need to call `handler.ExperimentalLogger`.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [47-47]
Correct the possessive form "its".
- and possibly it's dependencies.
+ and possibly its dependencies.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [53-53]
Correct the spelling of "order".
- in orderr to enable globals you can set `ENABLE_EXPERIMENTAL_ZAP_GLOBALS` to `true` in your environment.
+ in order to enable globals you can set `ENABLE_EXPERIMENTAL_ZAP_GLOBALS` to `true` in your environment.
core/metrics/README.md
Outdated
| | `METRICS_PATH` | Path to serve metrics on | `/metrics` | | ||
| | Enviornment Variable | Description | Default | | ||
| |------------------------|-----------------------------------------------------------------------------|------------| | ||
| | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` | |
There was a problem hiding this comment.
Correct the spelling of "Whether".
- | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` |
+ | `METRICS_PORT_ENABLED` | Whether or not to enable the metrics endpoint. | `true` |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.
| | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` | | |
| | `METRICS_PORT_ENABLED` | Whether or not to enable the metrics endpoint. | `true` | |
core/metrics/README.md
Outdated
| | `METRICS_PORT_ENABLED` | Wether or not to enable the metrics endpoint. | `true` | | ||
| | `METRICS_PORT` | Port to serve metrics on. | `8080` | | ||
| | `METRICS_PATH` | Path to serve metrics on | `/metrics` | | ||
| | `METRICS_ALWAYS_SYNC` | Wether or not to wait until metrics have been exported before ending a span | `false` | |
There was a problem hiding this comment.
Correct the spelling of "Whether".
- | `METRICS_ALWAYS_SYNC` | Wether or not to wait until metrics have been exported before ending a span | `false` |
+ | `METRICS_ALWAYS_SYNC` | Whether or not to wait until metrics have been exported before ending a span | `false` |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.
| | `METRICS_ALWAYS_SYNC` | Wether or not to wait until metrics have been exported before ending a span | `false` | | |
| | `METRICS_ALWAYS_SYNC` | Whether or not to wait until metrics have been exported before ending a span | `false` | |
core/metrics/base.go
Outdated
| if core.GetEnvBool("METRICS_ALWAYS_SYNC", false) { | ||
| extraOpts = append(extraOpts, tracesdk.WithSampler(tracesdk.AlwaysSample())) | ||
| } |
There was a problem hiding this comment.
Ensure the environment variable METRICS_ALWAYS_SYNC is documented and explained clearly in the README or relevant documentation.
Would you like assistance in updating the documentation to include details about the METRICS_ALWAYS_SYNC environment variable?
trajan0x
force-pushed
the
submitter/dynamic-tx-fees
branch
from
19efa8f
to
85969eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (7)
- ethergo/submitter/chain_queue.go (1 hunks)
- ethergo/submitter/db/mocks/service.go (1 hunks)
- ethergo/submitter/db/service.go (1 hunks)
- ethergo/submitter/db/txdb/store.go (1 hunks)
- ethergo/submitter/submitter.go (1 hunks)
- services/rfq/relayer/inventory/circle.go (4 hunks)
- services/rfq/relayer/service/chainindexer.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- ethergo/submitter/chain_queue.go
- ethergo/submitter/db/mocks/service.go
- ethergo/submitter/db/service.go
- ethergo/submitter/db/txdb/store.go
Additional Context Used
GitHub Check Runs (2)
codecov/patch success (8)
services/rfq/relayer/inventory/circle.go: [warning] 336-336: services/rfq/relayer/inventory/circle.go#L336
Added line #L336 was not covered by tests
services/rfq/relayer/inventory/circle.go: [warning] 365-365: services/rfq/relayer/inventory/circle.go#L365
Added line #L365 was not covered by tests
services/rfq/relayer/inventory/circle.go: [warning] 388-388: services/rfq/relayer/inventory/circle.go#L388
Added line #L388 was not covered by tests
services/rfq/relayer/inventory/circle.go: [warning] 405-405: services/rfq/relayer/inventory/circle.go#L405
Added line #L405 was not covered by tests
services/rfq/relayer/service/chainindexer.go: [warning] 208-208: services/rfq/relayer/service/chainindexer.go#L208
Added line #L208 was not covered by testsLint (ethergo) failure (4)
ethergo/submitter/submitter.go: [failure] 431-431:
commentFormatting: put a space between//and comment text (gocritic)
| @@ -205,7 +205,7 @@ | |||
| func (r *Relayer) handleDepositClaimed(ctx context.Context, event *fastbridge.FastBridgeBridgeDepositClaimed, chainID int) error { | |||
| err := r.inventory.Rebalance(ctx, chainID, event.Token) | |||
| if err != nil { | |||
| logger.Errorf("could not rebalance: %w", err) | |||
| logger.Errorf("could not rebalance: %v", err) | |||
There was a problem hiding this comment.
Consider adding test coverage for the error logging change.
| @@ -333,7 +333,7 @@ | |||
|
|
|||
| event, err := parser.ParseDepositForBurn(log) | |||
| if err != nil { | |||
| logger.Warnf("could not parse circle request sent: %w", err) | |||
| logger.Warnf("could not parse circle request sent: %v", err) | |||
There was a problem hiding this comment.
Consider adding test coverage for the error logging change.
| @@ -362,7 +362,7 @@ | |||
| } | |||
| err = c.db.UpdateRebalance(ctx, rebalanceModel, true) | |||
| if err != nil { | |||
| logger.Warnf("could not update rebalance status: %w", err) | |||
| logger.Warnf("could not update rebalance status: %v", err) | |||
There was a problem hiding this comment.
Consider adding test coverage for the error logging change.
| @@ -385,7 +385,7 @@ | |||
|
|
|||
| parsedEvent, err := parser.ParseMessageReceived(log) | |||
| if err != nil { | |||
| logger.Warnf("could not parse circle request fulfilled: %w", err) | |||
| logger.Warnf("could not parse circle request fulfilled: %v", err) | |||
There was a problem hiding this comment.
Consider adding test coverage for the error logging change.
| @@ -402,7 +402,7 @@ | |||
| } | |||
| err = c.db.UpdateRebalance(ctx, rebalanceModel, false) | |||
| if err != nil { | |||
| logger.Warnf("could not update rebalance status: %w", err) | |||
| logger.Warnf("could not update rebalance status: %v", err) | |||
There was a problem hiding this comment.
Consider adding test coverage for the error logging change.
ethergo/submitter/submitter.go
Outdated
| // gas tip cap can actually be zero. | ||
| //if transactor.GasTipCap == nil || transactor.GasTipCap.Cmp(big.NewInt(0)) == 0 { | ||
| // transactor.GasTipCap = t.config.GetBaseGasPrice(chainID) | ||
| //} |
There was a problem hiding this comment.
Revisit the decision to comment out the GasTipCap setting, as it may impact gas price handling for EIP-1559 transactions.
There was a problem hiding this comment.
Actionable comments posted: 4
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- services/rfq/relayer/service/handlers.go (1 hunks)
Additional comments not posted (10)
services/rfq/relayer/service/handlers.go (10)
292-297: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Consider adding package-level documentation to describe the purpose and functionality of the
servicepackage.
292-297: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Add documentation for the
handleBridgeRequestedLogfunction to explain its role and the process it initiates.
292-297: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Consider using a more robust logging framework that supports structured logging. This can improve the readability and searchability of logs.
292-297: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Evaluate the error handling strategy to ensure that it provides enough context for debugging and does not expose sensitive information.
292-297: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Ensure that all external calls, especially those interacting with blockchain clients and databases, are adequately protected against potential failures and have retry mechanisms where appropriate.
292-297: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
For functions interacting with blockchain contracts, consider implementing caching for frequently accessed data to reduce the number of external calls and improve performance.
292-297: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Review the concurrency model used in the application, especially regarding database access and blockchain interactions, to prevent data races and ensure thread safety.
Verification successful
Given the extensive use of Go routines across the codebase, including in key application files, a deeper inspection of specific implementations is necessary to verify the concurrency model and thread safety, especially regarding database access and blockchain interactions. To proceed, we'll focus on inspecting the implementation of Go routines in "services/rfq/relayer/service/relayer.go" and "services/rfq/relayer/inventory/manager.go" for thread safety mechanisms.
The provided code from
relayer.goandmanager.gounder theservices/rfq/relayer/serviceandservices/rfq/relayer/inventorydirectories respectively, offers a comprehensive view into the concurrency model and thread safety mechanisms employed in the application, particularly in relation to database access and blockchain interactions.Analysis of
relayer.go:
- The
Relayerstruct uses anerrgroup.Groupto manage concurrent operations, which is a common and effective pattern in Go for handling multiple goroutines and their potential errors in a synchronized manner.- The
Startmethod of theRelayerstruct launches several goroutines usingg.Go(func() error {...})for different tasks such as starting chain indexers, managing a claim cache, submitting quotes, running a DB selector, starting a submitter, running an API server, and starting an inventory manager. This indicates a heavy reliance on concurrency for various operations.- The use of
context.Contextanderrgroup.Grouptogether ensures that the goroutines are cancellable and their errors are collectively managed, which is a good practice for maintaining thread safety and managing concurrency effectively.- The
inventoryManagerImplstruct inmanager.godemonstrates a sophisticated use of concurrency for managing inventory, including mutexes (sync.RWMutex) for thread-safe access to shared resources, and error groups (errgroup.Group) for managing concurrent operations.- The
refreshBalancesmethod inmanager.gouses async.WaitGroupto wait for multiple goroutines that refresh token balances concurrently, indicating an understanding of how to manage concurrent tasks and their completion.Conclusion:
The application employs a variety of concurrency patterns and synchronization mechanisms, such as goroutines, mutexes, error groups, and wait groups, which are indicative of a thoughtful approach to ensuring thread safety and managing concurrent access to shared resources like the database and blockchain interactions. The use of
context.Contextacross goroutines further emphasizes a design that considers cancellation and timeout handling, which are crucial for robust concurrency management.Given the evidence from the code snippets provided, it can be concluded that the application has implemented a concurrency model with mechanisms aimed at preventing data races and ensuring thread safety, especially in critical areas involving database access and blockchain interactions.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential data race conditions in Go routines and channel usage. rg --type go "go func"Length of output: 12545
Script:
#!/bin/bash # Inspect the implementation of Go routines in relayer.go and manager.go for thread safety mechanisms fd "relayer.go" services/rfq/relayer/service --exec cat {} fd "manager.go" services/rfq/relayer/inventory --exec cat {}Length of output: 40274
292-297: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Optimize the use of context within the application, ensuring that it is passed correctly through function calls and used effectively for cancellation and timeouts.
292-297: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Adopt a consistent error handling strategy across the application to improve maintainability and readability.
292-297: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Review the use of magic numbers and strings throughout the code, replacing them with named constants where appropriate to improve readability and maintainability.
| return fmt.Errorf("could not get bridge status: %w", err) | ||
| } | ||
|
|
||
| if bs == fastbridge.RelayerProved.Int() { | ||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.ClaimPending) | ||
| if err != nil { | ||
| return fmt.Errorf("could not update request status: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| if bs == fastbridge.RelayerClaimed.Int() { | ||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.ClaimCompleted) | ||
| if err != nil { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Replace the TODO comments with actual implementations or create issues in your project's issue tracker to address them later. This ensures that important tasks are not forgotten.
Would you like me to help by creating GitHub issues for these TODO items?
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Consider abstracting blockchain interaction logic into a separate package or module to improve modularity and facilitate easier testing.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Implement unit tests for critical functions, especially those involving complex logic or external interactions, to ensure reliability and facilitate future changes.
Would you like me to help by creating unit tests for some of these functions?
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [14-14]
Ensure that all TODO comments are either addressed or tracked in your project's issue tracker to prevent them from being overlooked.
Would you like assistance in converting these TODOs into GitHub issues?
trajan0x
force-pushed
the
submitter/dynamic-tx-fees
branch
from
a85e92f
to
21abb18
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- ethergo/chain/gas/cmp.go (1 hunks)
- ethergo/submitter/submitter.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- ethergo/chain/gas/cmp.go
ethergo/submitter/submitter.go
Outdated
| maxHit := false | ||
| if transactor.GasPrice != nil && maxPrice.Cmp(transactor.GasPrice) < 0 { | ||
| transactor.GasPrice = maxPrice | ||
| maxHit = true | ||
| } | ||
| if transactor.GasFeeCap != nil && maxPrice.Cmp(transactor.GasFeeCap) < 0 { | ||
| transactor.GasFeeCap = maxPrice | ||
| maxHit = true | ||
| } | ||
|
|
||
| if maxHit { | ||
| span.AddEvent("ceiling hit", trace.WithAttributes(attribute.String("max_price", maxPrice.String()))) |
There was a problem hiding this comment.
Consider adding a log statement when the gas price ceiling is hit to improve observability.
if maxHit {
+ logger.Warn("Gas price ceiling hit", "max_price", maxPrice.String())
span.AddEvent("ceiling hit", trace.WithAttributes(attribute.String("max_price", maxPrice.String())))
}This will help in monitoring and debugging scenarios where the gas price ceiling is frequently reached, providing immediate visibility into potential issues with gas price configuration or network conditions.
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.
| maxHit := false | |
| if transactor.GasPrice != nil && maxPrice.Cmp(transactor.GasPrice) < 0 { | |
| transactor.GasPrice = maxPrice | |
| maxHit = true | |
| } | |
| if transactor.GasFeeCap != nil && maxPrice.Cmp(transactor.GasFeeCap) < 0 { | |
| transactor.GasFeeCap = maxPrice | |
| maxHit = true | |
| } | |
| if maxHit { | |
| span.AddEvent("ceiling hit", trace.WithAttributes(attribute.String("max_price", maxPrice.String()))) | |
| maxHit := false | |
| if transactor.GasPrice != nil && maxPrice.Cmp(transactor.GasPrice) < 0 { | |
| transactor.GasPrice = maxPrice | |
| maxHit = true | |
| } | |
| if transactor.GasFeeCap != nil && maxPrice.Cmp(transactor.GasFeeCap) < 0 { | |
| transactor.GasFeeCap = maxPrice | |
| maxHit = true | |
| } | |
| if maxHit { | |
| logger.Warn("Gas price ceiling hit", "max_price", maxPrice.String()) | |
| span.AddEvent("ceiling hit", trace.WithAttributes(attribute.String("max_price", maxPrice.String()))) |
ethergo/submitter/submitter.go
Outdated
| var marketFeeCap, marketTipCap, marketGasPrice *big.Int | ||
| // TODO: cache both of these values | ||
| shouldBump := true | ||
| if t.config.SupportsEIP1559(int(bigChainID.Uint64())) { | ||
| transactor.GasFeeCap, err = client.SuggestGasPrice(ctx) | ||
| marketFeeCap, err = client.SuggestGasPrice(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get gas price: %w", err) | ||
| } | ||
| // don't bump fee cap if we hit the max configured gas price | ||
| if transactor.GasFeeCap.Cmp(maxPrice) > 0 { | ||
| transactor.GasFeeCap = maxPrice | ||
| shouldBump = false | ||
| span.AddEvent("not bumping fee cap since max price is reached") | ||
| } | ||
|
|
||
| transactor.GasTipCap, err = client.SuggestGasTipCap(ctx) | ||
| marketTipCap, err = client.SuggestGasTipCap(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get gas tip cap: %w", err) | ||
| } | ||
| } else { | ||
| transactor.GasPrice, err = client.SuggestGasPrice(ctx) | ||
| marketGasPrice, err = client.SuggestGasPrice(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get gas price: %w", err) | ||
| } | ||
| } | ||
| t.applyBaseGasPrice(transactor, chainID) | ||
|
|
||
| //nolint: nestif | ||
| if prevTx != nil && shouldBump { | ||
| if prevTx != nil { | ||
| gasBlock, err := t.getGasBlock(ctx, client, chainID) | ||
| if err != nil { | ||
| span.AddEvent("could not get gas block", trace.WithAttributes(attribute.String("error", err.Error()))) | ||
| return err | ||
| } | ||
|
|
||
| // if the prev tx was greater than this one, we should bump the gas price from that point | ||
| comparison := gas.CompareGas(prevTx, gas.OptsToComparableTx(transactor), gasBlock.BaseFee) | ||
| if comparison > 0 { | ||
| if prevTx.Type() == types.LegacyTxType { | ||
| transactor.GasPrice = core.CopyBigInt(prevTx.GasPrice()) | ||
| } else { | ||
| transactor.GasTipCap = core.CopyBigInt(prevTx.GasTipCap()) | ||
| transactor.GasFeeCap = core.CopyBigInt(prevTx.GasFeeCap()) | ||
| } | ||
| } | ||
| gas.BumpGasFees(transactor, t.config.GetGasBumpPercentage(chainID), gasBlock.BaseFee, maxPrice) | ||
| } else { | ||
| // in the case where this is our first attempt, use market prices!. | ||
| if t.config.SupportsEIP1559(chainID) { | ||
| transactor.GasFeeCap = new(big.Int).Mul(marketFeeCap, big.NewInt(2)) | ||
| // use 1 wei if tip cap is zero | ||
| transactor.GasTipCap = bigMax(marketTipCap, big.NewInt(1)) | ||
| } else { | ||
| transactor.GasPrice = marketGasPrice | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // bigMax returns the larger of two big ints. | ||
| func bigMax(a, b *big.Int) *big.Int { | ||
| if a.Cmp(b) > 0 { | ||
| return a | ||
| } | ||
| return b |
There was a problem hiding this comment.
Refactor the market-based gas price calculation to a separate method to improve modularity and readability.
- var marketFeeCap, marketTipCap, marketGasPrice *big.Int
- // TODO: cache both of these values
- if t.config.SupportsEIP1559(int(bigChainID.Uint64())) {
- marketFeeCap, err = client.SuggestGasPrice(ctx)
- if err != nil {
- return fmt.Errorf("could not get gas price: %w", err)
- }
- marketTipCap, err = client.SuggestGasTipCap(ctx)
- if err != nil {
- return fmt.Errorf("could not get gas tip cap: %w", err)
- }
- } else {
- marketGasPrice, err = client.SuggestGasPrice(ctx)
- if err != nil {
- return fmt.Errorf("could not get gas price: %w", err)
- }
- }
+ marketFeeCap, marketTipCap, marketGasPrice, err := t.getMarketBasedGasPrices(ctx, client, bigChainID)
+ if err != nil {
+ return err
+ }
+ // New method implementation
+ func (t *txSubmitterImpl) getMarketBasedGasPrices(ctx context.Context, client client.EVM, bigChainID *big.Int) (marketFeeCap, marketTipCap, marketGasPrice *big.Int, err error) {
+ if t.config.SupportsEIP1559(int(bigChainID.Uint64())) {
+ marketFeeCap, err = client.SuggestGasPrice(ctx)
+ if err != nil {
+ return nil, nil, nil, fmt.Errorf("could not get gas price: %w", err)
+ }
+ marketTipCap, err = client.SuggestGasTipCap(ctx)
+ if err != nil {
+ return nil, nil, nil, fmt.Errorf("could not get gas tip cap: %w", err)
+ }
+ } else {
+ marketGasPrice, err = client.SuggestGasPrice(ctx)
+ if err != nil {
+ return nil, nil, nil, fmt.Errorf("could not get gas price: %w", err)
+ }
+ }
+ return marketFeeCap, marketTipCap, marketGasPrice, nil
+ }Extracting the logic for fetching market-based gas prices into a separate method enhances the code's structure, making it easier to understand and maintain. Additionally, it opens up possibilities for caching these values more effectively in the future, as hinted by the existing TODO comment.
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.
| var marketFeeCap, marketTipCap, marketGasPrice *big.Int | |
| // TODO: cache both of these values | |
| shouldBump := true | |
| if t.config.SupportsEIP1559(int(bigChainID.Uint64())) { | |
| transactor.GasFeeCap, err = client.SuggestGasPrice(ctx) | |
| marketFeeCap, err = client.SuggestGasPrice(ctx) | |
| if err != nil { | |
| return fmt.Errorf("could not get gas price: %w", err) | |
| } | |
| // don't bump fee cap if we hit the max configured gas price | |
| if transactor.GasFeeCap.Cmp(maxPrice) > 0 { | |
| transactor.GasFeeCap = maxPrice | |
| shouldBump = false | |
| span.AddEvent("not bumping fee cap since max price is reached") | |
| } | |
| transactor.GasTipCap, err = client.SuggestGasTipCap(ctx) | |
| marketTipCap, err = client.SuggestGasTipCap(ctx) | |
| if err != nil { | |
| return fmt.Errorf("could not get gas tip cap: %w", err) | |
| } | |
| } else { | |
| transactor.GasPrice, err = client.SuggestGasPrice(ctx) | |
| marketGasPrice, err = client.SuggestGasPrice(ctx) | |
| if err != nil { | |
| return fmt.Errorf("could not get gas price: %w", err) | |
| } | |
| } | |
| t.applyBaseGasPrice(transactor, chainID) | |
| //nolint: nestif | |
| if prevTx != nil && shouldBump { | |
| if prevTx != nil { | |
| gasBlock, err := t.getGasBlock(ctx, client, chainID) | |
| if err != nil { | |
| span.AddEvent("could not get gas block", trace.WithAttributes(attribute.String("error", err.Error()))) | |
| return err | |
| } | |
| // if the prev tx was greater than this one, we should bump the gas price from that point | |
| comparison := gas.CompareGas(prevTx, gas.OptsToComparableTx(transactor), gasBlock.BaseFee) | |
| if comparison > 0 { | |
| if prevTx.Type() == types.LegacyTxType { | |
| transactor.GasPrice = core.CopyBigInt(prevTx.GasPrice()) | |
| } else { | |
| transactor.GasTipCap = core.CopyBigInt(prevTx.GasTipCap()) | |
| transactor.GasFeeCap = core.CopyBigInt(prevTx.GasFeeCap()) | |
| } | |
| } | |
| gas.BumpGasFees(transactor, t.config.GetGasBumpPercentage(chainID), gasBlock.BaseFee, maxPrice) | |
| } else { | |
| // in the case where this is our first attempt, use market prices!. | |
| if t.config.SupportsEIP1559(chainID) { | |
| transactor.GasFeeCap = new(big.Int).Mul(marketFeeCap, big.NewInt(2)) | |
| // use 1 wei if tip cap is zero | |
| transactor.GasTipCap = bigMax(marketTipCap, big.NewInt(1)) | |
| } else { | |
| transactor.GasPrice = marketGasPrice | |
| } | |
| } | |
| return nil | |
| } | |
| // bigMax returns the larger of two big ints. | |
| func bigMax(a, b *big.Int) *big.Int { | |
| if a.Cmp(b) > 0 { | |
| return a | |
| } | |
| return b | |
| marketFeeCap, marketTipCap, marketGasPrice, err := t.getMarketBasedGasPrices(ctx, client, bigChainID) | |
| if err != nil { | |
| return err | |
| } | |
| t.applyBaseGasPrice(transactor, chainID) | |
| //nolint: nestif | |
| if prevTx != nil { | |
| gasBlock, err := t.getGasBlock(ctx, client, chainID) | |
| if err != nil { | |
| span.AddEvent("could not get gas block", trace.WithAttributes(attribute.String("error", err.Error()))) | |
| return err | |
| } | |
| gas.BumpGasFees(transactor, t.config.GetGasBumpPercentage(chainID), gasBlock.BaseFee, maxPrice) | |
| } else { | |
| // in the case where this is our first attempt, use market prices!. | |
| if t.config.SupportsEIP1559(chainID) { | |
| transactor.GasFeeCap = new(big.Int).Mul(marketFeeCap, big.NewInt(2)) | |
| // use 1 wei if tip cap is zero | |
| transactor.GasTipCap = bigMax(marketTipCap, big.NewInt(1)) | |
| } else { | |
| transactor.GasPrice = marketGasPrice | |
| } | |
| } | |
| return nil | |
| } | |
| func (t *txSubmitterImpl) getMarketBasedGasPrices(ctx context.Context, client client.EVM, bigChainID *big.Int) (marketFeeCap, marketTipCap, marketGasPrice *big.Int, err error) { | |
| if t.config.SupportsEIP1559(int(bigChainID.Uint64())) { | |
| marketFeeCap, err = client.SuggestGasPrice(ctx) | |
| if err != nil { | |
| return nil, nil, nil, fmt.Errorf("could not get gas price: %w", err) | |
| } | |
| marketTipCap, err = client.SuggestGasTipCap(ctx) | |
| if err != nil { | |
| return nil, nil, nil, fmt.Errorf("could not get gas tip cap: %w", err) | |
| } | |
| } else { | |
| marketGasPrice, err = client.SuggestGasPrice(ctx) | |
| if err != nil { | |
| return nil, nil, nil, fmt.Errorf("could not get gas price: %w", err) | |
| } | |
| } | |
| return marketFeeCap, marketTipCap, marketGasPrice, nil | |
| } | |
| // bigMax returns the larger of two big ints. | |
| func bigMax(a, b *big.Int) *big.Int { | |
| if a.Cmp(b) > 0 { | |
| return a | |
| } | |
| return b |
ethergo/submitter/submitter.go
Outdated
| if prevTx != nil { | ||
| gasBlock, err := t.getGasBlock(ctx, client, chainID) | ||
| if err != nil { | ||
| span.AddEvent("could not get gas block", trace.WithAttributes(attribute.String("error", err.Error()))) | ||
| return err | ||
| } | ||
|
|
||
| // if the prev tx was greater than this one, we should bump the gas price from that point | ||
| comparison := gas.CompareGas(prevTx, gas.OptsToComparableTx(transactor), gasBlock.BaseFee) | ||
| if comparison > 0 { | ||
| if prevTx.Type() == types.LegacyTxType { | ||
| transactor.GasPrice = core.CopyBigInt(prevTx.GasPrice()) | ||
| } else { | ||
| transactor.GasTipCap = core.CopyBigInt(prevTx.GasTipCap()) | ||
| transactor.GasFeeCap = core.CopyBigInt(prevTx.GasFeeCap()) | ||
| } | ||
| } | ||
| gas.BumpGasFees(transactor, t.config.GetGasBumpPercentage(chainID), gasBlock.BaseFee, maxPrice) | ||
| } else { | ||
| // in the case where this is our first attempt, use market prices!. | ||
| if t.config.SupportsEIP1559(chainID) { | ||
| transactor.GasFeeCap = new(big.Int).Mul(marketFeeCap, big.NewInt(2)) | ||
| // use 1 wei if tip cap is zero | ||
| transactor.GasTipCap = bigMax(marketTipCap, big.NewInt(1)) | ||
| } else { | ||
| transactor.GasPrice = marketGasPrice | ||
| } |
There was a problem hiding this comment.
Ensure that the multiplication factor for marketFeeCap is configurable.
if t.config.SupportsEIP1559(chainID) {
- transactor.GasFeeCap = new(big.Int).Mul(marketFeeCap, big.NewInt(2))
+ multiplier := t.config.GetMarketFeeCapMultiplier(chainID)
+ transactor.GasFeeCap = new(big.Int).Mul(marketFeeCap, big.NewInt(multiplier))
transactor.GasTipCap = bigMax(marketTipCap, big.NewInt(1))
} else {
transactor.GasPrice = marketGasPrice
}Hardcoding the multiplier for marketFeeCap as 2 may not be suitable for all network conditions or configurations. Making this value configurable allows for more flexibility and adaptability to different environments and requirements.
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.
| if prevTx != nil { | |
| gasBlock, err := t.getGasBlock(ctx, client, chainID) | |
| if err != nil { | |
| span.AddEvent("could not get gas block", trace.WithAttributes(attribute.String("error", err.Error()))) | |
| return err | |
| } | |
| // if the prev tx was greater than this one, we should bump the gas price from that point | |
| comparison := gas.CompareGas(prevTx, gas.OptsToComparableTx(transactor), gasBlock.BaseFee) | |
| if comparison > 0 { | |
| if prevTx.Type() == types.LegacyTxType { | |
| transactor.GasPrice = core.CopyBigInt(prevTx.GasPrice()) | |
| } else { | |
| transactor.GasTipCap = core.CopyBigInt(prevTx.GasTipCap()) | |
| transactor.GasFeeCap = core.CopyBigInt(prevTx.GasFeeCap()) | |
| } | |
| } | |
| gas.BumpGasFees(transactor, t.config.GetGasBumpPercentage(chainID), gasBlock.BaseFee, maxPrice) | |
| } else { | |
| // in the case where this is our first attempt, use market prices!. | |
| if t.config.SupportsEIP1559(chainID) { | |
| transactor.GasFeeCap = new(big.Int).Mul(marketFeeCap, big.NewInt(2)) | |
| // use 1 wei if tip cap is zero | |
| transactor.GasTipCap = bigMax(marketTipCap, big.NewInt(1)) | |
| } else { | |
| transactor.GasPrice = marketGasPrice | |
| } | |
| if prevTx != nil { | |
| gasBlock, err := t.getGasBlock(ctx, client, chainID) | |
| if err != nil { | |
| span.AddEvent("could not get gas block", trace.WithAttributes(attribute.String("error", err.Error()))) | |
| return err | |
| } | |
| gas.BumpGasFees(transactor, t.config.GetGasBumpPercentage(chainID), gasBlock.BaseFee, maxPrice) | |
| } else { | |
| // in the case where this is our first attempt, use market prices!. | |
| if t.config.SupportsEIP1559(chainID) { | |
| multiplier := t.config.GetMarketFeeCapMultiplier(chainID) | |
| transactor.GasFeeCap = new(big.Int).Mul(marketFeeCap, big.NewInt(multiplier)) | |
| transactor.GasTipCap = bigMax(marketTipCap, big.NewInt(1)) | |
| } else { | |
| transactor.GasPrice = marketGasPrice | |
| } |
trajan0x
force-pushed
the
submitter/dynamic-tx-fees
branch
from
c1b5d31
to
0d98056
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- ethergo/submitter/submitter.go (1 hunks)
Additional Context Used
GitHub Check Runs (1)
Lint (ethergo) failure (4)
ethergo/submitter/submitter.go: [failure] 431-431:
commentFormatting: put a space between//and comment text (gocritic)
Additional comments not posted (1)
ethergo/submitter/submitter.go (1)
430-433: Revisit the decision to comment out theGasTipCapsetting, as it may impact gas price handling for EIP-1559 transactions.
This change might have been made to align with specific gas pricing strategies, but it's crucial to ensure that it doesn't inadvertently affect transaction processing under EIP-1559 conditions.
ethergo/submitter/submitter.go
Outdated
| transactor.GasTipCap = t.config.GetBaseGasPrice(chainID) | ||
| } | ||
| // gas tip cap can actually be zero. | ||
| //if transactor.GasTipCap == nil || transactor.GasTipCap.Cmp(big.NewInt(0)) == 0 { |
There was a problem hiding this comment.
Format the comment correctly by adding a space after //.
- //if transactor.GasTipCap == nil || transactor.GasTipCap.Cmp(big.NewInt(0)) == 0 {
+ // if transactor.GasTipCap == nil || transactor.GasTipCap.Cmp(big.NewInt(0)) == 0 {This change aligns with Go's standard comment formatting practices.
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.
| //if transactor.GasTipCap == nil || transactor.GasTipCap.Cmp(big.NewInt(0)) == 0 { | |
| // if transactor.GasTipCap == nil || transactor.GasTipCap.Cmp(big.NewInt(0)) == 0 { |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- ethergo/chain/gas/cmp.go (1 hunks)
- ethergo/submitter/submitter.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- ethergo/chain/gas/cmp.go
- ethergo/submitter/submitter.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- ethergo/chain/gas/cmp.go (3 hunks)
- ethergo/submitter/chain_queue.go (2 hunks)
- ethergo/submitter/submitter.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- ethergo/chain/gas/cmp.go
- ethergo/submitter/chain_queue.go
- ethergo/submitter/submitter.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- ethergo/chain/gas/cmp.go (1 hunks)
- ethergo/submitter/submitter.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- ethergo/chain/gas/cmp.go
- ethergo/submitter/submitter.go
There was a problem hiding this comment.
Actionable comments posted: 3
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- ethergo/submitter/submitter.go (2 hunks)
Additional Context Used
GitHub Check Runs (1)
Lint (ethergo) failure (4)
ethergo/submitter/submitter.go: [failure] 378-378:
if useEIP1559has complex nested blocks (complexity: 5) (nestif)
Additional comments not posted (1)
ethergo/submitter/submitter.go (1)
408-457: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [377-432]
Consider refactoring the
setGasPricemethod to reduce its complexity and improve readability.- // nolint: cyclop - // TODO: use options.The method
setGasPricehas grown in complexity, particularly with the addition of logic for handling EIP-1559 transactions and ensuring a minimum gas price. Refactoring this method could involve breaking it down into smaller, more focused functions or using a strategy pattern for different gas price setting behaviors. This would make the code easier to understand and maintain, especially as further adjustments to gas pricing logic may be required in the future.
* Feat: initial refactor of setGasPrice() * Feat: add more tracing * Cleanup: comments * Feat: BaseGasPrice -> MinGasPrice * Cleanup: remove applyMinGasPrice() * Feat: initial test cases * Feat: add WithOracleOverride cases * Feat: add PrevTx cases * Feat: add assertGasValues() helper * Cleanup: pass useDynamic to applyGasFloor() * Feat: remove old submitter test * Cleanup: lint * [goreleaser] * Cleanup: populateFromPrevTx() -> bumpFromPrevTx() * Feat: cap GasTipCap by GasFeeCap * [goreleaser] * Feat: default min gas price to 1 gwei * [goreleaser] * Feat: add minTipCap var * Fix: minTipCap = 10 wei * Cleanup: comment * [goreleaser] * Fix: test
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (5)
- ethergo/chain/gas/cmp.go (1 hunks)
- ethergo/submitter/config/config.go (4 hunks)
- ethergo/submitter/config/iconfig_generated.go (2 hunks)
- ethergo/submitter/submitter.go (2 hunks)
- ethergo/submitter/submitter_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- ethergo/chain/gas/cmp.go
- ethergo/submitter/submitter.go
Additional comments not posted (8)
ethergo/submitter/config/iconfig_generated.go (2)
18-19: RenamingGetBaseGasPricetoGetMinGasPriceclarifies the purpose of the method. Ensure all implementations ofIConfigare updated to reflect this change.
38-39: RenamingSetBaseGasPricetoSetMinGasPricealigns with the new gas pricing strategy. Verify that all calls to this method are updated across the codebase.ethergo/submitter/config/config.go (4)
34-35: Renaming the struct field fromBaseGasPricetoMinGasPriceinChainConfigis consistent with the interface changes. Ensure all usages ofChainConfigare updated accordingly.
70-71: RenamingDefaultBaseGasPricetoDefaultMinGasPricealigns with the new gas pricing strategy. Verify that all references to this constant are updated across the codebase.
113-123: The implementation ofGetMinGasPricecorrectly reflects the renaming. Ensure this method is used correctly wherever gas price calculations are performed.
220-222: TheSetMinGasPricemethod correctly updates theMinGasPricefield. Verify that all calls to this method are updated across the codebase.ethergo/submitter/submitter_test.go (2)
9-9: The addition of theparamsimport is necessary for accessing Ethereum parameters in the test cases.
33-205: The test cases for setting and bumping gas prices are comprehensive and cover a variety of scenarios. Ensure that the logic for handling gas prices, especially in the context of minimum and maximum values and transaction types, is thoroughly tested.
|
@coderabbitai review |
aureliusbtc
changed the title
Description
Misc fixes for #2410
Summary by CodeRabbit
METRICS_ALWAYS_SYNCto control metric export behavior.METRICS_ALWAYS_SYNCenvironment variable.MarkAllBeforeOrAtNonceReplacedOrConfirmedtoMarkAllBeforeNonceReplacedOrConfirmed.