RFQ Quoter: generate quotes in parallel #2847
Conversation
WalkthroughThe updates to Changes
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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Introduced parallel processing for quote generation using goroutines
- Added mutex for synchronization during quote generation
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2847 +/- ##
===================================================
+ Coverage 25.66758% 25.84121% +0.17362%
===================================================
Files 770 713 -57
Lines 55424 52811 -2613
Branches 80 80
===================================================
- Hits 14226 13647 -579
+ Misses 39740 37769 -1971
+ Partials 1458 1395 -63
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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/quoter/quoter.go (2 hunks)
Additional context used
GitHub Check: Lint (services/rfq)
services/rfq/relayer/quoter/quoter.go
[failure] 339-339:
Using the variable on range scopekeyTokenIDin function literal (scopelint)
[failure] 343-343:
Using the variable on range scopekeyTokenIDin function literal (scopelint)
Additional comments not posted (4)
services/rfq/relayer/quoter/quoter.go (4)
11-12: Concurrency and synchronization mechanisms added.The addition of
syncandsync/atomicpackages is appropriate for managing concurrency and synchronization.
Line range hint
84-84:
Concurrency using errgroup added.The use of
errgroupto screen sender and recipient addresses in parallel is an excellent addition for improving performance.
Line range hint
245-245:
Concurrency and error handling improvements.The changes to handle concurrency and error handling are well-implemented, ensuring efficient and reliable quote submission.
329-331: Concurrency using errgroup added.The use of
errgroupto generate quotes in parallel is an excellent addition for improving performance.
| if quoteErr != nil { | ||
| // continue generating quotes even if one fails | ||
| span.AddEvent("error generating quote", trace.WithAttributes( | ||
| attribute.String("key_token_id", keyTokenID), |
There was a problem hiding this comment.
Fix variable scope issue in goroutine.
The variable keyTokenID from the range scope is used in the goroutine, which can lead to incorrect behavior.
- span.AddEvent("error generating quote", trace.WithAttributes(
- attribute.String("key_token_id", keyTokenID),
+ span.AddEvent("error generating quote", trace.WithAttributes(
+ attribute.String("key_token_id", keyToken),Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Lint (services/rfq)
[failure] 343-343:
Using the variable on range scopekeyTokenIDin function literal (scopelint)
| g.Go(func() error { | ||
| quote, quoteErr := m.generateQuote(gctx, keyTokenID, chainID, address, balance, destRFQAddr) |
There was a problem hiding this comment.
Fix variable scope issue in goroutine.
The variable keyTokenID from the range scope is used in the goroutine, which can lead to incorrect behavior.
- for _, tokenID := range itemTokenIDs {
- if tokenID == destTokenID {
- g.Go(func() error {
- quote, quoteErr := m.generateQuote(gctx, keyTokenID, chainID, address, balance, destRFQAddr)
+ for i := range itemTokenIDs {
+ if itemTokenIDs[i] == destTokenID {
+ keyToken := keyTokenID
+ g.Go(func() error {
+ quote, quoteErr := m.generateQuote(gctx, keyToken, chainID, address, balance, destRFQAddr)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.
| g.Go(func() error { | |
| quote, quoteErr := m.generateQuote(gctx, keyTokenID, chainID, address, balance, destRFQAddr) | |
| for i := range itemTokenIDs { | |
| if itemTokenIDs[i] == destTokenID { | |
| keyToken := keyTokenID | |
| g.Go(func() error { | |
| quote, quoteErr := m.generateQuote(gctx, keyToken, chainID, address, balance, destRFQAddr) |
Tools
GitHub Check: Lint (services/rfq)
[failure] 339-339:
Using the variable on range scopekeyTokenIDin function literal (scopelint)
Deploying sanguine-fe with
|
| Latest commit: |
1c09d00
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7ab16090.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-parallel-quote-gen.sanguine-fe.pages.dev |
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Modified
services/rfq/relayer/quoter/quoter.goto generate quotes in parallel using goroutines and an error group - Enhanced error handling during quote generation for increased robustness
- Improved performance and reliability of the quote generation process
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/quoter/quoter.go (2 hunks)
Additional comments not posted (7)
services/rfq/relayer/quoter/quoter.go (7)
11-11: Imports approved.The new imports for synchronization and parallel processing are appropriate.
Line range hint
48-58:
Struct additions approved.The added fields for managing relay state and recording metrics are appropriate.
Line range hint
75-99:
Function changes approved.The initialization of
relayPausedandquoteAmountGaugeis appropriate.
Line range hint
106-134:
Function changes approved.The use of
relayPausedanderrgroupfor parallel processing enhances error handling and efficiency.
Line range hint
184-208:
Function changes approved.The use of
relayPausedfor error management is appropriate.
Line range hint
213-248:
Function changes approved.The use of
errgroupfor parallel quote generation enhances efficiency.
329-360: Function changes approved.The use of
errgroupfor parallel quote generation andsync.Mutexfor synchronization enhances efficiency and thread safety.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
rfqRefundcommand incontrib/opbot/botmd/commands.gofor refunding quote requests. - Updated
contrib/opbot/botmd/botmd.goto initialize RPC client, signer, and transaction submitter. - Introduced new configuration fields in
contrib/opbot/config/config.gofor RFQ API, OmniRPC, Signer, Submitter, and Database. - Enhanced tracing middleware in
contrib/opbot/botmd/middleware.gofor better context propagation. - Updated
services/rfq/api/rest/server.goto add a new API endpoint for retrieving contract addresses.
45 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Summary by CodeRabbit
New Features
Bug Fixes