Submitter: only bump gas price for transactions with current nonce #2417
Conversation
WalkthroughThe recent updates focus on refining gas price management for Ethereum transactions. Changes include introducing a parameter for adjusting gas prices based on transaction nonce and modifying test expectations for the Changes
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 (
|
dwasse
changed the title
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- ethergo/submitter/chain_queue.go (3 hunks)
- ethergo/submitter/submitter_test.go (1 hunks)
Additional Context Used
Additional comments not posted (3)
ethergo/submitter/chain_queue.go (2)
87-92: The logic to conditionally bump the gas price based on the current nonce of the transaction is clear and aligns with the PR's objective. However, ensure that thecurrentNoncevariable accurately reflects the latest nonce at the time of this operation, considering potential race conditions or updates that may occur in a multi-threaded environment.
176-186: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [152-183]
The modification to the
bumpTXfunction, introducing abumpGasPriceboolean to control whether the gas price should be bumped, is a sensible approach to optimize gas expenditure. However, consider the following points for improvement and verification:
- Error Handling: Ensure comprehensive error handling within this function, especially for external calls like
getGasEstimate,GetTransactor, andsetGasPrice. It appears to be handled well, but double-check that all possible error scenarios are covered.- Gas Price Calculation Logic: The logic to decide whether to bump the gas price or not (
prevTx = nilif!bumpGasPrice) is straightforward. However, verify that this approach does not introduce any edge cases where a transaction might get stuck due to insufficient gas price in a rapidly changing gas price environment.- Testing: It's crucial to have extensive tests covering various scenarios, including edge cases where the gas price fluctuates significantly. Ensure that tests cover the new logic introduced for handling
bumpGasPrice.ethergo/submitter/submitter_test.go (1)
47-47: The adjustment in the expected number of invocations for theSuggestGasPricefunction fromTwice()to3is consistent with the changes made in thechain_queue.gofile. This reflects an understanding of how the modified logic affects the behavior of the system under test. However, ensure that this change is accompanied by a comprehensive test case that specifically validates the new behavior introduced in the PR, including scenarios where transactions have the current nonce and where they do not.
Deploying sanguine-fe with
|
| Latest commit: |
0bd0e22
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f39e0a3f.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-bump-current-nonce.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2417 +/- ##
===================================================
- Coverage 46.72382% 46.58246% -0.14137%
===================================================
Files 396 406 +10
Lines 30447 30797 +350
Branches 83 132 +49
===================================================
+ Hits 14226 14346 +120
- Misses 14691 14922 +231
+ Partials 1530 1529 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yaml
Files ignored due to path filters (4)
services/cctp-relayer/go.modis excluded by!**/*.modservices/cctp-relayer/go.sumis excluded by!**/*.sumservices/rfq/go.modis excluded by!**/*.modservices/rfq/go.sumis excluded by!**/*.sum
Files selected for processing (11)
- packages/contracts-rfq/CHANGELOG.md (1 hunks)
- packages/contracts-rfq/deployments/base/.chainId (1 hunks)
- packages/contracts-rfq/deployments/base/FastBridge.json (1 hunks)
- packages/contracts-rfq/foundry.toml (1 hunks)
- packages/contracts-rfq/package.json (1 hunks)
- packages/contracts-rfq/script/fb-config.sh (1 hunks)
- packages/contracts-rfq/script/fb-deploy.sh (1 hunks)
- services/cctp-relayer/relayer/options.go (1 hunks)
- services/cctp-relayer/relayer/relayer.go (2 hunks)
- services/rfq/relayer/inventory/synapse.go (2 hunks)
- services/rfq/relayer/service/relayer.go (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/contracts-rfq/package.json
Additional Context Used
GitHub Check Runs (1)
codecov/patch success (4)
services/cctp-relayer/relayer/options.go: [warning] 14-17: services/cctp-relayer/relayer/options.go#L14-L17
Added lines #L14 - L17 were not covered by tests
services/cctp-relayer/relayer/options.go: [warning] 23-24: services/cctp-relayer/relayer/options.go#L23-L24
Added lines #L23 - L24 were not covered by tests
Additional comments not posted (10)
packages/contracts-rfq/deployments/base/.chainId (1)
1-1: The addition of the chain ID8453aligns with the PR objectives of deploying contracts to a new base network. Looks good!packages/contracts-rfq/script/fb-deploy.sh (1)
17-17: The addition of the deployment command for thebasenetwork aligns with the PR objectives and follows bash scripting best practices. Looks good!packages/contracts-rfq/script/fb-config.sh (1)
18-18: The addition of the configuration command for thebasenetwork aligns with the PR objectives and follows bash scripting best practices. Looks good!packages/contracts-rfq/foundry.toml (1)
21-21: The addition of thebasenetwork fields under[rpc_endpoints]and[etherscan]aligns with the PR objectives of supporting a new base network and follows the TOML format correctly. Looks good!Also applies to: 27-27
packages/contracts-rfq/CHANGELOG.md (1)
6-11: The documentation of the deployment of RFQ to Base in version 0.2.0 accurately reflects the PR's objectives. The changelog is clear and follows conventional commit guidelines. Looks good!services/rfq/relayer/inventory/synapse.go (1)
6-6: The update in import statements, replacing theexplorercontracts import with one forcctp-relayercontracts, aligns with the PR objectives and improves code modularity. Looks good!services/rfq/relayer/service/relayer.go (1)
277-277: The addition of therelayer.WithSubmitter(r.submitter)parameter to theNewCCTPRelayerfunction call aligns with the PR objectives and likely enhances the functionality of the CCTP relayer by providing it with a submitter for transactions. Looks good!services/cctp-relayer/relayer/relayer.go (2)
77-80: The addition ofrawOpts ...OptionsArgsOptionas a variadic argument in theNewCCTPRelayerfunction introduces a flexible way to configure theCCTPRelayer. This approach allows for future extensions without changing the function signature. However, ensure that the documentation is updated to reflect this change and provide examples of how to use these options.
117-120: The conditional initialization oftxSubmitterbased on whether it is provided throughoptsis a good practice for dependency injection, which can facilitate testing and customization. However, it's crucial to ensure that the default behavior (whentxSubmitterisnil) is well-documented and tested to prevent any unexpected behavior.packages/contracts-rfq/deployments/base/FastBridge.json (1)
1-1219: TheFastBridge.jsonfile provides a comprehensive ABI for the FastBridge contract, including functions for bridging, claiming, disputing, and managing roles. Ensure that the contract's implementation aligns with these definitions, especially regarding security and efficiency in gas usage. Additionally, verify that the contract has undergone thorough testing, including unit tests and integration tests, to ensure its correctness and security.
| func WithSubmitter(txSubmitter submitter.TransactionSubmitter) OptionsArgsOption { | ||
| return func(options *relayerOptions) { | ||
| options.submitter = txSubmitter | ||
| } |
There was a problem hiding this comment.
The addition of the WithSubmitter option follows the pattern of options design in Go and aligns with the PR objectives. However, it's recommended to add test coverage for this new functionality to ensure its correctness and maintainability.
Would you like assistance in creating test cases for the new WithSubmitter option?
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Description
Only supply a
prevTxtosetGasPrice()in the case we are on the current nonce. This should help conserve "gas expenditure" as far as mempool processing is concerned.Metadata
Summary by CodeRabbit
FastBridgewith various transaction bridging functions.foundry.tomlconfig file with new fields forbasenetwork.FastBridgecontract version to 0.2.0.basenetwork in deployment scripts.synapse.gofor accurate contract references.startCCTPRelayerfunction for proper relayer configuration.