RFQ Relayer: restrict state transitions #2787
Conversation
WalkthroughThe recent updates involve integrating a logger for the Changes
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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
- Added state transition validation to
UpdateQuoteRequestStatusinquote.go - Introduced
prevStatusparameter toUpdateQuoteRequestStatusindb.go - Updated
UpdateQuoteRequestStatuscalls inchainindexer.gowith new parameter - Ensured proper state management in
handlers.goby including previous status - Modified
UpdateQuoteRequestStatusinrelayer.goandstatushandler.goto include current status
6 file(s) reviewed, no comment(s)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2787 +/- ##
===================================================
+ Coverage 25.57487% 25.64578% +0.07090%
===================================================
Files 766 766
Lines 54925 54972 +47
Branches 80 80
===================================================
+ Hits 14047 14098 +51
+ Misses 39428 39427 -1
+ Partials 1450 1447 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
b140cb2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1e17f5b9.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-restrict-state-tx.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: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- services/rfq/relayer/reldb/base/quote.go (3 hunks)
- services/rfq/relayer/reldb/db.go (1 hunks)
- services/rfq/relayer/service/chainindexer.go (4 hunks)
- services/rfq/relayer/service/handlers.go (12 hunks)
- services/rfq/relayer/service/relayer.go (1 hunks)
- services/rfq/relayer/service/statushandler.go (1 hunks)
Additional context used
GitHub Check: Build (1.22.x, ubuntu-latest)
services/rfq/relayer/service/handlers.go
[failure] 378-378:
undefined: request
Additional comments not posted (5)
services/rfq/relayer/reldb/base/quote.go (1)
17-18: Introduced global logger for relayer-dbAdding a logger is a good practice for tracking application behavior and errors, which is crucial for debugging and monitoring.
services/rfq/relayer/service/chainindexer.go (1)
88-88: Ensure consistency in error handling forUpdateQuoteRequestStatuscallsAll calls to
UpdateQuoteRequestStatusin this file passnilforprevStatus. This could lead to unintended behavior if theprevStatusis required for accurate validation. Verify that this is the intended usage or consider passing the actual previous status where necessary.Also applies to: 102-102, 116-116, 209-209
services/rfq/relayer/reldb/db.go (1)
27-27: Updated function signature to includeprevStatusThe function
UpdateQuoteRequestStatusnow requires aprevStatusparameter. This change aligns with the need for more robust state transition checks as indicated in the summary. Ensure all calls to this function are updated accordingly.services/rfq/relayer/service/statushandler.go (1)
141-141: Handling of deadline exceeded statusThe middleware correctly updates the status to
DeadlineExceededwhen the current time surpasses the deadline. This is a critical update for maintaining the integrity of the quote request lifecycle. Ensure that theprevStatusis correctly utilized in the status update logic.services/rfq/relayer/service/relayer.go (1)
394-394: Proper handling of expired deadlines inprocessRequestThe method checks if the quote request has expired and updates its status accordingly. This is crucial for preventing actions on outdated requests. However, ensure that
prevStatusis appropriately used to prevent incorrect state transitions.
| func isValidStateTransition(prevStatus, status reldb.QuoteRequestStatus) bool { | ||
| if status == reldb.DeadlineExceeded || status == reldb.WillNotProcess { | ||
| return true | ||
| } | ||
| return status > prevStatus | ||
| } |
There was a problem hiding this comment.
Validation logic in isValidStateTransition seems limited
The function checks if the new status is greater than the previous status or matches specific statuses. This might be too restrictive or not adequately capture valid transitions, depending on business rules. If more complex rules are needed, consider expanding this logic or using a state machine library.
| func (s Store) UpdateQuoteRequestStatus(ctx context.Context, id [32]byte, status reldb.QuoteRequestStatus, prevStatus *reldb.QuoteRequestStatus) error { | ||
| if prevStatus == nil { | ||
| req, err := s.GetQuoteRequestByID(ctx, id) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get quote: %w", err) | ||
| } | ||
| prevStatus = &req.Status | ||
| } | ||
| if !isValidStateTransition(*prevStatus, status) { | ||
| logger.Warnf("invalid state transition from %s to %s [txid=%s]", *prevStatus, status, hexutil.Encode(id[:])) | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Review the logic for state transition validation and error handling
The method UpdateQuoteRequestStatus now includes logic to handle state transitions validation. While the use of prevStatus to fetch the current status if not provided is logical, the function returns nil when an invalid state transition is detected, which might silently ignore errors. Consider returning an error instead to make the failure explicit.
- return nil
+ return fmt.Errorf("invalid state transition from %s to %s [txid=%s]", *prevStatus, status, hexutil.Encode(id[:]))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.
| func (s Store) UpdateQuoteRequestStatus(ctx context.Context, id [32]byte, status reldb.QuoteRequestStatus, prevStatus *reldb.QuoteRequestStatus) error { | |
| if prevStatus == nil { | |
| req, err := s.GetQuoteRequestByID(ctx, id) | |
| if err != nil { | |
| return fmt.Errorf("could not get quote: %w", err) | |
| } | |
| prevStatus = &req.Status | |
| } | |
| if !isValidStateTransition(*prevStatus, status) { | |
| logger.Warnf("invalid state transition from %s to %s [txid=%s]", *prevStatus, status, hexutil.Encode(id[:])) | |
| return nil | |
| } | |
| func (s Store) UpdateQuoteRequestStatus(ctx context.Context, id [32]byte, status reldb.QuoteRequestStatus, prevStatus *reldb.QuoteRequestStatus) error { | |
| if prevStatus == nil { | |
| req, err := s.GetQuoteRequestByID(ctx, id) | |
| if err != nil { | |
| return fmt.Errorf("could not get quote: %w", err) | |
| } | |
| prevStatus = &req.Status | |
| } | |
| if !isValidStateTransition(*prevStatus, status) { | |
| logger.Warnf("invalid state transition from %s to %s [txid=%s]", *prevStatus, status, hexutil.Encode(id[:])) | |
| return fmt.Errorf("invalid state transition from %s to %s [txid=%s]", *prevStatus, status, hexutil.Encode(id[:])) | |
| } |
| prevStatus = &req.Status | ||
| } | ||
| if !isValidStateTransition(*prevStatus, status) { | ||
| logger.Warnf("invalid state transition from %s to %s [txid=%s]", *prevStatus, status, hexutil.Encode(id[:])) |
There was a problem hiding this comment.
Chose not to return error here since we presumably don't want to panic on this, and checking errors.Is() on the result introduces a bit of code bloat
There was a problem hiding this comment.
Completely understand the hesitation to do this, but in this case I'll be renaming the method to UpdateQuoteRequestStatusIFValid or somethign.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Enhanced logging for quote request status updates
- Added validation for state transitions during quote request status updates
- Updated functions to include previous status parameter for quote request status updates
- Introduced Prometheus metrics setup in RFQ API server
- Added new indirect dependency on
github.com/cornelk/hashmap
29 file(s) reviewed, no comment(s)
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Enhanced logging for quote request status updates
- Added validation for state transitions during quote request status updates
- Improved handling of quote request statuses by passing previous status information
- Updated functions to include an additional parameter for previous status in quote request status updates
No file(s) reviewed, no comment(s)
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Enhanced logging for quote request status updates
- Added validation for state transitions during quote request status updates
- Introduced
Keys()method tountypedMapMutexinterface for debugging - Replaced
relayMtxwithhandlerMtxfor state transition handling - Updated functions to include previous status information for quote requests
7 file(s) reviewed, 1 comment(s)
core/mapmutex/mapmutex_test.go
Outdated
| @@ -11,6 +11,7 @@ import ( | |||
| "github.com/ethereum/go-ethereum/common" | |||
| . "github.com/stretchr/testify/assert" | |||
| "github.com/synapsecns/sanguine/core/mapmutex" | |||
| "gotest.tools/assert" | |||
There was a problem hiding this comment.
🪶 style: Redundant import of gotest.tools/assert. Consider using only one assertion library for consistency.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- core/mapmutex/mapmutex.go (3 hunks)
- core/mapmutex/mapmutex_test.go (2 hunks)
- core/mapmutex/stringer.go (5 hunks)
- services/rfq/relayer/service/chainindexer.go (2 hunks)
- services/rfq/relayer/service/handlers.go (13 hunks)
- services/rfq/relayer/service/relayer.go (3 hunks)
- services/rfq/relayer/service/statushandler.go (5 hunks)
Files skipped from review as they are similar to previous changes (3)
- services/rfq/relayer/service/chainindexer.go
- services/rfq/relayer/service/relayer.go
- services/rfq/relayer/service/statushandler.go
Additional context used
GitHub Check: Lint (core)
core/mapmutex/stringer.go
[failure] 32-32:
type assertion must be checked (forcetypeassert)
[failure] 77-77:
type assertion must be checked (forcetypeassert)
[failure] 107-107:
type assertion must be checked (forcetypeassert)
GitHub Check: Build (1.22.x, ubuntu-latest)
services/rfq/relayer/service/handlers.go
[failure] 378-378:
undefined: request
Additional comments not posted (6)
core/mapmutex/stringer.go (3)
11-11: New method added toStringerMapMutexinterface.The addition of the
Keys()method is consistent with the changes in other mutex interfaces to provide a way to retrieve all keys. This is useful for debugging and monitoring.
48-48: New method added toStringMapMutexinterface.The
Keys()method has been consistently added across different mutex interfaces, enhancing the utility and consistency of the API.
86-86: New method added toIntMapMutexinterface.Adding the
Keys()method allows for consistent functionality across all mutex types, facilitating easier management and debugging.core/mapmutex/mapmutex.go (1)
17-17: New method added tountypedMapMutexinterface.The addition of the
Keys()method is a useful enhancement for debugging and monitoring purposes, allowing access to current keys in the mutex map.core/mapmutex/mapmutex_test.go (1)
56-76: New test cases forKeys()method across different mutex implementations.These tests are well-structured and provide coverage for the new functionality. They check both the content and the size of the keys list, which is crucial for verifying the correct behavior of the
Keys()method.services/rfq/relayer/service/handlers.go (1)
145-145: Review of UpdatedUpdateQuoteRequestStatusCallsAll the updates to
UpdateQuoteRequestStatusfunction calls across various functions seem to correctly pass the newprevStatusparameter where needed. This aligns with the PR's objective to enhance state transition validations by passing the previous status into the update function.However, there are a couple of issues and improvements to note:
Error Handling and Logging: Ensure that all errors are handled appropriately and logged. This is crucial for debugging and maintaining the system, especially in a blockchain context where transactions are irreversible.
Static Analysis Issue:
- Line 378: The variable
requestis undefined in the context ofhandleProofProvided. This is likely a copy-paste error or an oversight. This needs a fix to use the correct variable from the function's scope.- err = r.db.UpdateQuoteRequestStatus(ctx, req.TransactionId, reldb.ProvePosted, &request.Status) + err = r.db.UpdateQuoteRequestStatus(ctx, req.TransactionId, reldb.ProvePosted, &req.Status)Consistency Check:
- General: For functions where
prevStatusis set tonil, consider whether this is intentional or if it should be fetching the current status from the database or another source. This could potentially lead to incorrect state transitions if not handled correctly.Performance Consideration:
- General: When updating statuses in a blockchain context, consider the implications of transaction costs and delays. Efficient handling of these updates can prevent unnecessary costs and improve the responsiveness of the system.
Security Checks:
- General: Ensure that all status updates are authorized and validated to prevent unauthorized state changes, which could be critical in a financial or blockchain system.
Overall, the changes align well with the objectives of the PR, but the highlighted issues and considerations should be addressed to ensure robustness and security.
Also applies to: 170-170, 182-182, 209-209, 273-273, 303-303, 335-335, 364-364, 378-378, 411-411, 446-446, 463-463
core/mapmutex/stringer.go
Outdated
| // Keys returns the keys of the map. | ||
| func (s stringMutexImpl) Keys() []string { | ||
| keys := []string{} | ||
| for _, key := range s.mapMux.Keys() { | ||
| keys = append(keys, key.(string)) | ||
| } | ||
| return keys | ||
| } |
There was a problem hiding this comment.
Implementation of Keys() in stringMutexImpl needs safe type assertion.
Similar to the stringerLockerImpl, this implementation also needs to handle type assertions safely to prevent potential runtime panics.
- keys = append(keys, key.(string))
+ strKey, ok := key.(string)
+ if !ok {
+ return nil, fmt.Errorf("type assertion to string failed")
+ }
+ keys = append(keys, strKey)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.
| // Keys returns the keys of the map. | |
| func (s stringMutexImpl) Keys() []string { | |
| keys := []string{} | |
| for _, key := range s.mapMux.Keys() { | |
| keys = append(keys, key.(string)) | |
| } | |
| return keys | |
| } | |
| // Keys returns the keys of the map. | |
| func (s stringMutexImpl) Keys() ([]string, error) { | |
| keys := []string{} | |
| for _, key := range s.mapMux.Keys() { | |
| strKey, ok := key.(string) | |
| if !ok { | |
| return nil, fmt.Errorf("type assertion to string failed") | |
| } | |
| keys = append(keys, strKey) | |
| } | |
| return keys, nil | |
| } |
Tools
GitHub Check: Lint (core)
[failure] 77-77:
type assertion must be checked (forcetypeassert)
core/mapmutex/stringer.go
Outdated
| // Keys returns the keys of the map. | ||
| func (s stringerLockerImpl) Keys() []string { | ||
| keys := []string{} | ||
| for _, key := range s.mapMux.Keys() { | ||
| keys = append(keys, key.(string)) | ||
| } | ||
| return keys | ||
| } |
There was a problem hiding this comment.
Implementation of Keys() in stringerLockerImpl needs safe type assertion.
The method uses a type assertion that could panic if the underlying type is not a string. This should be handled more safely.
- keys = append(keys, key.(string))
+ strKey, ok := key.(string)
+ if !ok {
+ return nil, fmt.Errorf("type assertion to string failed")
+ }
+ keys = append(keys, strKey)Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Lint (core)
[failure] 32-32:
type assertion must be checked (forcetypeassert)
core/mapmutex/stringer.go
Outdated
| // Keys returns the keys of the map. | ||
| func (i intMapMux) Keys() []int { | ||
| keys := []int{} | ||
| for _, key := range i.mapMux.Keys() { | ||
| keys = append(keys, key.(int)) | ||
| } | ||
| return keys | ||
| } |
There was a problem hiding this comment.
Implementation of Keys() in intMapMux needs safe type assertion.
This method also requires safe handling of type assertions to ensure robustness and prevent runtime errors.
- keys = append(keys, key.(int))
+ intKey, ok := key.(int)
+ if !ok {
+ return nil, fmt.Errorf("type assertion to int failed")
+ }
+ keys = append(keys, intKey)Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Lint (core)
[failure] 107-107:
type assertion must be checked (forcetypeassert)
core/mapmutex/mapmutex.go
Outdated
| // Keys returns all keys in the map. | ||
| func (m *untypedMapMutexImpl) Keys() []interface{} { | ||
| // note that we don't lock here in case the map is already locked. | ||
| // the idea is that this function can be used for debugging- | ||
| // there may be read inconsistencies. | ||
| keys := make([]interface{}, 0, len(m.ma)) | ||
| for k := range m.ma { | ||
| keys = append(keys, k) | ||
| } | ||
| return keys | ||
| } |
There was a problem hiding this comment.
Implementation of Keys() in untypedMapMutexImpl lacks synchronization.
The implementation does not lock the mutex map when accessing it, which can lead to race conditions and inconsistent reads. This should be addressed to ensure thread safety.
+ m.ml.Lock()
+ defer m.ml.Unlock()
keys := make([]interface{}, 0, len(m.ma))
for k := range m.ma {
keys = append(keys, k)
}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.
| // Keys returns all keys in the map. | |
| func (m *untypedMapMutexImpl) Keys() []interface{} { | |
| // note that we don't lock here in case the map is already locked. | |
| // the idea is that this function can be used for debugging- | |
| // there may be read inconsistencies. | |
| keys := make([]interface{}, 0, len(m.ma)) | |
| for k := range m.ma { | |
| keys = append(keys, k) | |
| } | |
| return keys | |
| } | |
| // Keys returns all keys in the map. | |
| func (m *untypedMapMutexImpl) Keys() []interface{} { | |
| // note that we don't lock here in case the map is already locked. | |
| // the idea is that this function can be used for debugging- | |
| // there may be read inconsistencies. | |
| m.ml.Lock() | |
| defer m.ml.Unlock() | |
| keys := make([]interface{}, 0, len(m.ma)) | |
| for k := range m.ma { | |
| keys = append(keys, k) | |
| } | |
| return keys | |
| } |
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Enhanced state validation for quote requests by including previous status in
UpdateQuoteRequestStatuscalls - Modified
UpdateQuoteRequestStatuscall forProvePostedto exclude previous status - Introduced logging in the
relayer-dbpackage for better tracking and debugging - Added methods to retrieve all keys from map mutexes for enhanced debugging
- Added test cases for key retrieval functionality in various map mutex types
1 file(s) reviewed, no comment(s)
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/service/handlers.go (13 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/service/handlers.go
core/mapmutex/mapmutex.go
Outdated
| @@ -82,6 +84,18 @@ func (m *untypedMapMutexImpl) TryLock(key interface{}) (Unlocker, bool) { | |||
| return nil, false | |||
| } | |||
|
|
|||
| // Keys returns all keys in the map. | |||
| func (m *untypedMapMutexImpl) Keys() []interface{} { | |||
| // note that we don't lock here in case the map is already locked. | |||
There was a problem hiding this comment.
@dwasse yeah this is just wrong imo.
This is definitely going to nil pointer- if it was only used for tests ifne but this method is unsafe b/c if could lead to iterating over a non-existent slice element
The lock here isn't global anyway and won't block anything else for long at all. The whole point of mapmutex is the Locks are not global even - the lock here is internal only
core/mapmutex/mapmutex.go
Outdated
| "sync" | ||
|
|
||
| "github.com/LK4d4/trylock" |
There was a problem hiding this comment.
Think we should remove this dep actually?
See: LK4D4/trylock#3 seems like this functionality is built into sync.Mutex since 1.18
(Random aside: we're not using a canonical url for this either. canonical import url is github.com/LK4D4
and we use github.com/LK4d4 (notice the casing difference:
I was curious about this since this seemed like a bug in x/pkgsite, but apparentely this is intended behavior when packages existed pre-go.mod:
See golang/go#45409
Apparently, this was a huge issue with logrus when they renamed the package from sirupsen/logrus to Sirupsen/logrus (see sirupsen/logrus#384 followed by sirupsen/logrus#570)
There was a problem hiding this comment.
okay, i've decided we should address this in a different pr (only bc this requires make tidy)
tracking in #2814
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)
- core/mapmutex/mapmutex.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/mapmutex/mapmutex.go
| prevStatus = &req.Status | ||
| } | ||
| if !isValidStateTransition(*prevStatus, status) { | ||
| logger.Warnf("invalid state transition from %s to %s [txid=%s]", *prevStatus, status, hexutil.Encode(id[:])) |
There was a problem hiding this comment.
Completely understand the hesitation to do this, but in this case I'll be renaming the method to UpdateQuoteRequestStatusIFValid or somethign.
trajan0x
force-pushed
the
feat/restrict-state-tx
branch
from
e867a9b
to
d5299e0
Compare
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Updated
.codeclimate.ymlto exclude generated files from analysis - Added
otelginmetricsdirectory to.golangci.ymlskip list - Conditional Signoz client initialization in
contrib/opbot/botmd/botmd.go - Introduced
requiresSignozmethod incontrib/opbot/botmd/commands.go - Added tracing and metrics middleware in
contrib/opbot/botmd/middleware.go
178 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced
Keys()method tocore/mapmutex/mapmutex.gofor key retrieval. - Added test cases for
Keys()method incore/mapmutex/mapmutex_test.go. - Enhanced logging and validation for quote request status transitions in
services/rfq/relayer/reldb/base/quote.go. - Updated
UpdateQuoteRequestStatuscalls inservices/rfq/relayer/service/handlers.goto include additional status arguments. - Removed span parameter from
handleNotEnoughInventoryinservices/rfq/relayer/service/handlers.go.
5 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Enhanced validation for quote request status transitions in
services/rfq/relayer/reldb/base/quote.go - Updated
UpdateQuoteRequestStatusmethod to include additional status arguments inservices/rfq/relayer/reldb/base/quote.go - Removed logging for invalid state transitions in
services/rfq/relayer/reldb/base/quote.go
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 19
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- core/mapmutex/mapmutex_test.go (1 hunks)
- core/mapmutex/stringer.go (5 hunks)
- services/rfq/relayer/reldb/base/quote.go (3 hunks)
- services/rfq/relayer/service/handlers.go (13 hunks)
Files skipped from review as they are similar to previous changes (2)
- core/mapmutex/mapmutex_test.go
- core/mapmutex/stringer.go
Additional context used
GitHub Check: codecov/patch
services/rfq/relayer/reldb/base/quote.go
[warning] 96-102: services/rfq/relayer/reldb/base/quote.go#L96-L102
Added lines #L96 - L102 were not covered by tests
[warning] 104-106: services/rfq/relayer/reldb/base/quote.go#L104-L106
Added lines #L104 - L106 were not covered by tests
[warning] 139-143: services/rfq/relayer/reldb/base/quote.go#L139-L143
Added lines #L139 - L143 were not covered by testsservices/rfq/relayer/service/handlers.go
[warning] 40-40: services/rfq/relayer/service/handlers.go#L40
Added line #L40 was not covered by tests
[warning] 145-145: services/rfq/relayer/service/handlers.go#L145
Added line #L145 was not covered by tests
[warning] 170-170: services/rfq/relayer/service/handlers.go#L170
Added line #L170 was not covered by tests
[warning] 182-182: services/rfq/relayer/service/handlers.go#L182
Added line #L182 was not covered by tests
[warning] 209-209: services/rfq/relayer/service/handlers.go#L209
Added line #L209 was not covered by tests
[warning] 273-273: services/rfq/relayer/service/handlers.go#L273
Added line #L273 was not covered by tests
[warning] 303-303: services/rfq/relayer/service/handlers.go#L303
Added line #L303 was not covered by tests
[warning] 335-335: services/rfq/relayer/service/handlers.go#L335
Added line #L335 was not covered by tests
[warning] 364-364: services/rfq/relayer/service/handlers.go#L364
Added line #L364 was not covered by tests
[warning] 378-378: services/rfq/relayer/service/handlers.go#L378
Added line #L378 was not covered by tests
[warning] 411-411: services/rfq/relayer/service/handlers.go#L411
Added line #L411 was not covered by tests
[warning] 446-446: services/rfq/relayer/service/handlers.go#L446
Added line #L446 was not covered by tests
[warning] 463-463: services/rfq/relayer/service/handlers.go#L463
Added line #L463 was not covered by tests
GitHub Check: Lint (services/rfq)
services/rfq/relayer/reldb/base/quote.go
[failure] 17-17:
loggeris unused (deadcode)
| func (s Store) UpdateQuoteRequestStatus(ctx context.Context, id [32]byte, status reldb.QuoteRequestStatus, prevStatus *reldb.QuoteRequestStatus) error { | ||
| if prevStatus == nil { | ||
| req, err := s.GetQuoteRequestByID(ctx, id) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get quote: %w", err) | ||
| } | ||
| prevStatus = &req.Status | ||
| } | ||
| if !isValidStateTransition(*prevStatus, status) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Validation logic in isValidStateTransition seems limited
The function checks if the new status is greater than the previous status or matches specific statuses. This might be too restrictive or not adequately capture valid transitions, depending on business rules. If more complex rules are needed, consider expanding this logic or using a state machine library.
Tools
GitHub Check: codecov/patch
[warning] 96-102: services/rfq/relayer/reldb/base/quote.go#L96-L102
Added lines #L96 - L102 were not covered by tests
[warning] 104-106: services/rfq/relayer/reldb/base/quote.go#L104-L106
Added lines #L104 - L106 were not covered by tests
| func (s Store) UpdateQuoteRequestStatus(ctx context.Context, id [32]byte, status reldb.QuoteRequestStatus, prevStatus *reldb.QuoteRequestStatus) error { | ||
| if prevStatus == nil { | ||
| req, err := s.GetQuoteRequestByID(ctx, id) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get quote: %w", err) | ||
| } | ||
| prevStatus = &req.Status |
There was a problem hiding this comment.
Missing test coverage
The added lines for the UpdateQuoteRequestStatus function and the isValidStateTransition function are not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Also applies to: 104-106, 139-143
Tools
GitHub Check: codecov/patch
[warning] 96-102: services/rfq/relayer/reldb/base/quote.go#L96-L102
Added lines #L96 - L102 were not covered by tests
| func isValidStateTransition(prevStatus, status reldb.QuoteRequestStatus) bool { | ||
| if status == reldb.DeadlineExceeded || status == reldb.WillNotProcess { | ||
| return true | ||
| } | ||
| return status >= prevStatus |
There was a problem hiding this comment.
Limited validation logic in isValidStateTransition
The function only checks if the new status is greater than the previous status or matches specific statuses. This might be too restrictive or not adequately capture valid transitions, depending on business rules. Consider expanding this logic or using a state machine library.
Tools
GitHub Check: codecov/patch
[warning] 139-143: services/rfq/relayer/reldb/base/quote.go#L139-L143
Added lines #L139 - L143 were not covered by tests
Missing test coverage
The isValidStateTransition function is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 139-143: services/rfq/relayer/reldb/base/quote.go#L139-L143
Added lines #L139 - L143 were not covered by tests
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/common/hexutil" | ||
| "github.com/synapsecns/sanguine/services/rfq/relayer/reldb" | ||
| "gorm.io/gorm" | ||
| "gorm.io/gorm/clause" | ||
| ) | ||
|
|
||
| var logger = log.Logger("relayer-db") |
There was a problem hiding this comment.
Unused logger
The logger variable is declared but not used in the file.
Consider using the logger to log relevant information or remove the unused variable.
Tools
GitHub Check: Lint (services/rfq)
[failure] 17-17:
loggeris unused (deadcode)
| if !isValidStateTransition(*prevStatus, status) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Review the logic for state transition validation and error handling
The method UpdateQuoteRequestStatus now includes logic to handle state transitions validation. While the use of prevStatus to fetch the current status if not provided is logical, the function returns nil when an invalid state transition is detected, which might silently ignore errors. Consider returning an error instead to make the failure explicit.
- return nil
+ return fmt.Errorf("invalid state transition from %s to %s [txid=%s]", *prevStatus, status, hexutil.Encode(id[:]))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 !isValidStateTransition(*prevStatus, status) { | |
| return nil | |
| } | |
| if !isValidStateTransition(*prevStatus, status) { | |
| return fmt.Errorf("invalid state transition from %s to %s [txid=%s]", *prevStatus, status, hexutil.Encode(id[:])) | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 104-106: services/rfq/relayer/reldb/base/quote.go#L104-L106
Added lines #L104 - L106 were not covered by tests
| @@ -361,7 +361,7 @@ | |||
| return fmt.Errorf("could not submit transaction: %w", err) | |||
| } | |||
|
|
|||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.ProvePosting) | |||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.ProvePosting, &request.Status) | |||
There was a problem hiding this comment.
Missing test coverage
The status update logic in handleRelayCompleted is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 364-364: services/rfq/relayer/service/handlers.go#L364
Added line #L364 was not covered by tests
| @@ -375,7 +375,7 @@ | |||
| func (r *Relayer) handleProofProvided(ctx context.Context, req *fastbridge.FastBridgeBridgeProofProvided) (err error) { | |||
| // TODO: this can still get re-orged | |||
| // ALso: we should make sure the previous status is ProvePosting | |||
| err = r.db.UpdateQuoteRequestStatus(ctx, req.TransactionId, reldb.ProvePosted) | |||
| err = r.db.UpdateQuoteRequestStatus(ctx, req.TransactionId, reldb.ProvePosted, nil) | |||
There was a problem hiding this comment.
Missing test coverage
The status update logic in handleProofProvided is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 378-378: services/rfq/relayer/service/handlers.go#L378
Added line #L378 was not covered by tests
| @@ -408,7 +408,7 @@ | |||
| } | |||
|
|
|||
| if bs == fastbridge.RelayerClaimed.Int() { | |||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.ClaimCompleted) | |||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.ClaimCompleted, &request.Status) | |||
There was a problem hiding this comment.
Missing test coverage
The status update logic in handleProofPosted is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 411-411: services/rfq/relayer/service/handlers.go#L411
Added line #L411 was not covered by tests
| @@ -443,7 +443,7 @@ | |||
| return fmt.Errorf("could not submit transaction: %w", err) | |||
| } | |||
|
|
|||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.ClaimPending) | |||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.ClaimPending, &request.Status) | |||
There was a problem hiding this comment.
Missing test coverage
The status update logic in handleProofPosted is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 446-446: services/rfq/relayer/service/handlers.go#L446
Added line #L446 was not covered by tests
| @@ -460,7 +460,7 @@ | |||
| } | |||
| // if committableBalance > destAmount | |||
| if committableBalance.Cmp(request.Transaction.DestAmount) > 0 { | |||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.CommittedPending) | |||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.CommittedPending, &request.Status) | |||
There was a problem hiding this comment.
Missing test coverage
The status update logic in handleNotEnoughInventory is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 463-463: services/rfq/relayer/service/handlers.go#L463
Added line #L463 was not covered by tests
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/reldb/base/quote.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/reldb/base/quote.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Removed anon.to URL redirection and added
slacker.WithUnfurlLinks(false)incontrib/opbot/botmd/commands.go - Updated
contrib/opbot/go.modto use a forked slacker package - Added
Blacklistfield toConfigstruct incontrib/screener-api/config/config.go - Renamed
blacklistCachetoblacklistincontrib/screener-api/screener/screener.go - Enhanced signal handling and context management in
core/commandline/shell.go
25 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)
- core/mapmutex/mapmutex.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/mapmutex/mapmutex.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Removed
github.com/LK4d4/trylockdependency from multiplego.modfiles - Replaced
trylock.Mutexwithsync.Mutexin/core/mapmutex/mapmutex.go - Added
Keysmethod tountypedMapMutexinterface in/core/mapmutex/mapmutex.go - Enhanced quote request status updates with validation and logging
- Added test cases for key retrieval functionality in map mutex types
12 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Summary by CodeRabbit
New Features
Bug Fixes
UpdateQuoteRequestStatusfunction to check for valid state transitions and log any invalid transitions.Tests