feat(rfq-relayer): insert record even for transactions with invalid tokens #3473
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
Deploying sanguine-fe with
|
| Latest commit: |
a741015
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://97b33e6d.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-relayer-reason.sanguine-fe.pages.dev |
![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
🧹 Nitpick comments (1)
services/rfq/relayer/service/handlers.go (1)
94-102: Validate dbReq fields more thoroughly before storage.The newly introduced dbReq is well structured; however, consider validating important fields such as Sender and Transaction to ensure that the subsequent logic can rely on properly formed data. This can help avoid storing partially invalid records in the database.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/rfq/relayer/reldb/base/model.go(3 hunks)services/rfq/relayer/reldb/db.go(1 hunks)services/rfq/relayer/service/handlers.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
services/rfq/relayer/service/handlers.go
109-109: S1039: unnecessary use of fmt.Sprintf
(gosimple)
118-118: printf: fmt.Sprintf does not support error-wrapping directive %w
(govet)
123-123: printf: non-constant format string in call to fmt.Errorf
(govet)
🪛 GitHub Check: Lint (services/rfq)
services/rfq/relayer/service/handlers.go
[failure] 109-109:
S1039: unnecessary use of fmt.Sprintf (gosimple)
[failure] 118-118:
printf: fmt.Sprintf does not support error-wrapping directive %w (govet)
[failure] 123-123:
printf: non-constant format string in call to fmt.Errorf (govet)
🔇 Additional comments (6)
services/rfq/relayer/service/handlers.go (2)
125-126: Good practice: usage of token decimals is consistent with prior checks.
These lines correctly assign the validated decimals after checking for errors, preventing invalid or nil pointer dereferences. This approach is safe and clear.
130-130: Check for potential database connection errors.
The fmt.Errorf usage is correct for error wrapping. Just ensure that any underlying database errors are properly handled/restored or retried elsewhere if needed, to avoid silent data loss.
services/rfq/relayer/reldb/db.go (1)
91-91: New Reason field improves traceability.
Introducing the Reason field is beneficial for error tracking. Ensure that any updates to it remain consistent with the domain logic and that user-facing messages do not inadvertently expose sensitive information.
services/rfq/relayer/reldb/base/model.go (3)
96-97: Protect sensitive or verbose reason strings.
Adding the Reason field to the database struct is valuable for diagnosing unprocessed transactions. Confirm that the reason content does not include PII, keys, or large data that could bloat the database.
142-142: Propagation of Reason in FromQuoteRequest.
Forwarding request.Reason ensures consistent error reporting across the system. Double-check that the reason remains accurate and does not get overwritten downstream.
231-231: Reason field consistency in ToQuoteRequest.
By setting r.Reason here, the round-trip conversion ensures the reason is preserved when going from the database model back to a QuoteRequest. This is consistent with the rest of the changes and maintains clarity.
| dbReq.Status = reldb.WillNotProcess | ||
| dbReq.Reason = fmt.Sprintf("invalid token (no contract code at address)") | ||
| err = r.db.StoreQuoteRequest(ctx, dbReq) | ||
| if err != nil { | ||
| return fmt.Errorf("could not store db request: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary use of fmt.Sprintf.
At line 109: fmt.Sprintf("invalid token (no contract code at address)") can be replaced with a direct string assignment for clarity and to address the static analysis suggestion about the unnecessary use of fmt.Sprintf.
- dbReq.Reason = fmt.Sprintf("invalid token (no contract code at address)")
+ dbReq.Reason = "invalid token (no contract code at address)"📝 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.
| dbReq.Status = reldb.WillNotProcess | |
| dbReq.Reason = fmt.Sprintf("invalid token (no contract code at address)") | |
| err = r.db.StoreQuoteRequest(ctx, dbReq) | |
| if err != nil { | |
| return fmt.Errorf("could not store db request: %w", err) | |
| } | |
| dbReq.Status = reldb.WillNotProcess | |
| dbReq.Reason = "invalid token (no contract code at address)" | |
| err = r.db.StoreQuoteRequest(ctx, dbReq) | |
| if err != nil { | |
| return fmt.Errorf("could not store db request: %w", err) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
109-109: S1039: unnecessary use of fmt.Sprintf
(gosimple)
🪛 GitHub Check: Lint (services/rfq)
[failure] 109-109:
S1039: unnecessary use of fmt.Sprintf (gosimple)
| dbReq.Status = reldb.WillNotProcess | ||
| dbReq.Reason = fmt.Sprintf("could not get decimals: %w", err) | ||
| err = r.db.StoreQuoteRequest(ctx, dbReq) | ||
| if err != nil { | ||
| return fmt.Errorf("could not store db request: %w", err) | ||
| } | ||
| return fmt.Errorf(dbReq.Reason) |
There was a problem hiding this comment.
Use error-wrapping directives appropriately.
• At line 118: “fmt.Sprintf("could not get decimals: %w", err)” doesn’t support “%w” in fmt.Sprintf. Use fmt.Errorf for correct wrapping instead.
• At line 123: Return an error directly using errors.New or similar if you want the stored reason to be used as-is. This avoids potential format string misinterpretation.
- dbReq.Reason = fmt.Sprintf("could not get decimals: %w", err)
+ dbReq.Reason = fmt.Sprintf("could not get decimals: %s", err)
...
- return fmt.Errorf(dbReq.Reason)
+ return errors.New(dbReq.Reason)📝 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.
| dbReq.Status = reldb.WillNotProcess | |
| dbReq.Reason = fmt.Sprintf("could not get decimals: %w", err) | |
| err = r.db.StoreQuoteRequest(ctx, dbReq) | |
| if err != nil { | |
| return fmt.Errorf("could not store db request: %w", err) | |
| } | |
| return fmt.Errorf(dbReq.Reason) | |
| dbReq.Status = reldb.WillNotProcess | |
| dbReq.Reason = fmt.Sprintf("could not get decimals: %s", err) | |
| err = r.db.StoreQuoteRequest(ctx, dbReq) | |
| if err != nil { | |
| return fmt.Errorf("could not store db request: %w", err) | |
| } | |
| return errors.New(dbReq.Reason) |
🧰 Tools
🪛 golangci-lint (1.62.2)
118-118: printf: fmt.Sprintf does not support error-wrapping directive %w
(govet)
123-123: printf: non-constant format string in call to fmt.Errorf
(govet)
🪛 GitHub Check: Lint (services/rfq)
[failure] 118-118:
printf: fmt.Sprintf does not support error-wrapping directive %w (govet)
[failure] 123-123:
printf: non-constant format string in call to fmt.Errorf (govet)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3473 +/- ##
===================================================
- Coverage 34.04255% 31.52087% -2.52168%
===================================================
Files 388 489 +101
Lines 24581 33803 +9222
===================================================
+ Hits 8368 10655 +2287
- Misses 15472 22134 +6662
- Partials 741 1014 +273
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
🧹 Nitpick comments (2)
services/rfq/relayer/service/handlers.go (2)
94-102: Consider using field names in struct initialization for better readability.While the current initialization works, using explicit field names would make the code more maintainable and less prone to errors when new fields are added.
dbReq := reldb.QuoteRequest{ - BlockNumber: req.Raw.BlockNumber, - RawRequest: req.Request, - TransactionID: req.TransactionId, - Sender: req.Sender, - Transaction: bridgeTx, - Status: reldb.Seen, - OriginTxHash: req.Raw.TxHash, + BlockNumber: req.Raw.BlockNumber, + RawRequest: req.Request, + TransactionID: req.TransactionId, + Sender: req.Sender, + Transaction: bridgeTx, + Status: reldb.Seen, + OriginTxHash: req.Raw.TxHash, + Reason: "", // explicitly initialize new field }
125-126: Document pointer dereferencing assumption.Add a comment to clarify that the nil check for decimals was performed in the previous error handling block. This helps prevent accidental removal of the nil check during future refactoring.
+// Safe to dereference as nil check was performed in previous error handling block dbReq.OriginTokenDecimals = *originDecimals dbReq.DestTokenDecimals = *destDecimals
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/rfq/relayer/service/handlers.go(1 hunks)
🔇 Additional comments (1)
services/rfq/relayer/service/handlers.go (1)
108-113: Remove unnecessary use of fmt.Sprintf.
The string is static and doesn't require formatting.
dbReq.Status = reldb.WillNotProcess
-dbReq.Reason = "invalid token (no contract code at address)"
+dbReq.Reason = "invalid token (no contract code at address)"| dbReq.Status = reldb.WillNotProcess | ||
| dbReq.Reason = fmt.Sprintf("could not get decimals: %s", err.Error()) | ||
| err = r.db.StoreQuoteRequest(ctx, dbReq) | ||
| if err != nil { | ||
| return fmt.Errorf("could not store db request: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling consistency.
The error handling can be improved in two ways:
- Avoid duplicating the error message between Reason and returned error
- Use consistent error wrapping patterns
dbReq.Status = reldb.WillNotProcess
-dbReq.Reason = fmt.Sprintf("could not get decimals: %s", err.Error())
+dbReq.Reason = fmt.Sprintf("could not get decimals: %v", err)
err = r.db.StoreQuoteRequest(ctx, dbReq)
if err != nil {
return fmt.Errorf("could not store db request: %w", err)
}
-return fmt.Errorf("could not get decimals: %w", err)
+return fmt.Errorf("%s: %w", dbReq.Reason, err)📝 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.
| dbReq.Status = reldb.WillNotProcess | |
| dbReq.Reason = fmt.Sprintf("could not get decimals: %s", err.Error()) | |
| err = r.db.StoreQuoteRequest(ctx, dbReq) | |
| if err != nil { | |
| return fmt.Errorf("could not store db request: %w", err) | |
| } | |
| dbReq.Status = reldb.WillNotProcess | |
| dbReq.Reason = fmt.Sprintf("could not get decimals: %v", err) | |
| err = r.db.StoreQuoteRequest(ctx, dbReq) | |
| if err != nil { | |
| return fmt.Errorf("could not store db request: %w", err) | |
| } | |
| return fmt.Errorf("%s: %w", dbReq.Reason, err) |
|
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. |
Summary by CodeRabbit
New Features
Reason, to capture the reason for unprocessed transactions in quote requests.Bug Fixes
Chores