-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFQ Relayer: restrict state transitions #2787
Changes from 11 commits
3245068
09614e1
fe03dfe
806a6d0
ab62a34
beb95ff
cafee78
82da838
cf1acb4
8b1a14f
8c8b7c0
d58097e
6928b16
46baad1
6b33520
d5299e0
e4e9aad
7b362d4
49bcf17
570158e
f0bc3cb
4aa6a82
b140cb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,8 +2,9 @@ package mapmutex | |||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/LK4d4/trylock" | ||||||||||||||||||||||||||||||||||||||||||||||||||
"sync" | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/LK4d4/trylock" | ||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// untypedMapMutex wraps a map of mutexes. Each key locks separately. | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -13,6 +14,7 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||
type untypedMapMutex interface { | ||||||||||||||||||||||||||||||||||||||||||||||||||
Lock(key interface{}) Unlocker | ||||||||||||||||||||||||||||||||||||||||||||||||||
TryLock(key interface{}) (Unlocker, bool) | ||||||||||||||||||||||||||||||||||||||||||||||||||
Keys() []interface{} | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
type untypedMapMutexImpl struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 |
||||||||||||||||||||||||||||||||||||||||||||||||||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implementation of 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// Unlock releases the lock for this entry. | ||||||||||||||||||||||||||||||||||||||||||||||||||
func (me *mentry) Unlock() { | ||||||||||||||||||||||||||||||||||||||||||||||||||
m := me.m | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🪶 style: Redundant import of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dwasse also a good take There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in e867a9b |
||
) | ||
|
||
// ExampleMapMutex provides an example implementation of a map mutex. | ||
|
@@ -52,6 +53,28 @@ func (s MapMutexSuite) TestExampleMapMutex() { | |
NotPanics(s.T(), ExampleStringMapMutex) | ||
} | ||
|
||
func (s MapMutexSuite) TestKeys() { | ||
s.T().Run("StringMapMutexKeys", func(t *testing.T) { | ||
mapMutex := mapmutex.NewStringMapMutex() | ||
mapMutex.Lock("lock1") | ||
assert.Equal(t, "lock1", mapMutex.Keys()[0]) | ||
assert.Equal(t, 1, len(mapMutex.Keys())) | ||
}) | ||
s.T().Run("StringerMapMutexKeys", func(t *testing.T) { | ||
mapMutex := mapmutex.NewStringerMapMutex() | ||
vitalik := common.HexToAddress("0xab5801a7d398351b8be11c439e05c5b3259aec9b") | ||
mapMutex.Lock(vitalik) | ||
assert.Equal(t, vitalik.String(), mapMutex.Keys()[0]) | ||
assert.Equal(t, 1, len(mapMutex.Keys())) | ||
}) | ||
s.T().Run("IntMapMutexKeys", func(t *testing.T) { | ||
mapMutex := mapmutex.NewIntMapMutex() | ||
mapMutex.Lock(1) | ||
assert.Equal(t, 1, mapMutex.Keys()[0]) | ||
assert.Equal(t, 1, len(mapMutex.Keys())) | ||
}) | ||
} | ||
|
||
func (s MapMutexSuite) TestMapMutex() { | ||
//nolint:gosec | ||
r := rand.New(rand.NewSource(42)) | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,11 +1,14 @@ | ||||||||||||||||||||||||||||||||||||||||||
package mapmutex | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
import "fmt" | ||||||||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// StringerMapMutex is an implementation of mapMutex for the fmt.Stringer conforming types. | ||||||||||||||||||||||||||||||||||||||||||
type StringerMapMutex interface { | ||||||||||||||||||||||||||||||||||||||||||
Lock(key fmt.Stringer) Unlocker | ||||||||||||||||||||||||||||||||||||||||||
TryLock(key fmt.Stringer) (Unlocker, bool) | ||||||||||||||||||||||||||||||||||||||||||
Keys() []string | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// stringerLockerImpl is the implementation of StringerMapMutex. | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -22,6 +25,15 @@ | |||||||||||||||||||||||||||||||||||||||||
return s.mapMux.Lock(key.String()) | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implementation of 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)
ToolsGitHub Check: Lint (core)
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// NewStringerMapMutex creates an initialized locker that locks on fmt.String. | ||||||||||||||||||||||||||||||||||||||||||
func NewStringerMapMutex() StringerMapMutex { | ||||||||||||||||||||||||||||||||||||||||||
return &stringerLockerImpl{ | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -33,6 +45,7 @@ | |||||||||||||||||||||||||||||||||||||||||
type StringMapMutex interface { | ||||||||||||||||||||||||||||||||||||||||||
Lock(key string) Unlocker | ||||||||||||||||||||||||||||||||||||||||||
TryLock(key string) (Unlocker, bool) | ||||||||||||||||||||||||||||||||||||||||||
Keys() []string | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// stringMutexImpl locks on a string type. | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -57,10 +70,20 @@ | |||||||||||||||||||||||||||||||||||||||||
return s.mapMux.TryLock(key) | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implementation of Similar to the - 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
Suggested change
ToolsGitHub Check: Lint (core)
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// IntMapMutex is a map mutex that allows locking on an int. | ||||||||||||||||||||||||||||||||||||||||||
type IntMapMutex interface { | ||||||||||||||||||||||||||||||||||||||||||
Lock(key int) Unlocker | ||||||||||||||||||||||||||||||||||||||||||
TryLock(key int) (Unlocker, bool) | ||||||||||||||||||||||||||||||||||||||||||
Keys() []int | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// intMapMux locks on an int. | ||||||||||||||||||||||||||||||||||||||||||
|
@@ -77,6 +100,15 @@ | |||||||||||||||||||||||||||||||||||||||||
return i.mapMux.Lock(key) | ||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implementation of 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)
ToolsGitHub Check: Lint (core)
|
||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||
// NewIntMapMutex creates a map mutex for locking on an integer. | ||||||||||||||||||||||||||||||||||||||||||
func NewIntMapMutex() IntMapMutex { | ||||||||||||||||||||||||||||||||||||||||||
return &intMapMux{ | ||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,13 +5,17 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
"errors" | ||||||||||||||||||||||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/ipfs/go-log" | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
"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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused logger The Consider using the logger to log relevant information or remove the unused variable. ToolsGitHub Check: Lint (services/rfq)
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// StoreQuoteRequest stores a quote request. | ||||||||||||||||||||||||||||||||||||||||||||||||||
func (s Store) StoreQuoteRequest(ctx context.Context, request reldb.QuoteRequest) error { | ||||||||||||||||||||||||||||||||||||||||||||||||||
rq := FromQuoteRequest(request) | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -89,7 +93,19 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// UpdateQuoteRequestStatus todo: db test. | ||||||||||||||||||||||||||||||||||||||||||||||||||
func (s Store) UpdateQuoteRequestStatus(ctx context.Context, id [32]byte, status reldb.QuoteRequestStatus) error { | ||||||||||||||||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+92
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The added lines for the 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 ToolsGitHub Check: codecov/patch
|
||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chose not to return error here since we presumably don't want to panic on this, and checking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Completely understand the hesitation to do this, but in this case I'll be renaming the method to |
||||||||||||||||||||||||||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Review the logic for state transition validation and error handling The method - return nil
+ return fmt.Errorf("invalid state transition from %s to %s [txid=%s]", *prevStatus, status, hexutil.Encode(id[:])) Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
tx := s.DB().WithContext(ctx).Model(&RequestForQuote{}). | ||||||||||||||||||||||||||||||||||||||||||||||||||
Where(fmt.Sprintf("%s = ?", transactionIDFieldName), hexutil.Encode(id[:])). | ||||||||||||||||||||||||||||||||||||||||||||||||||
Update(statusFieldName, status) | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -120,3 +136,10 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
return nil | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validation logic in 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,7 +37,7 @@ | |
metrics.EndSpanWithErr(span, err) | ||
}() | ||
|
||
unlocker, ok := r.relayMtx.TryLock(hexutil.Encode(req.TransactionId[:])) | ||
unlocker, ok := r.handlerMtx.TryLock(hexutil.Encode(req.TransactionId[:])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The lock acquisition logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if !ok { | ||
span.SetAttributes(attribute.Bool("locked", true)) | ||
// already processing this request | ||
|
@@ -142,7 +142,7 @@ | |
return fmt.Errorf("could not determine if should process: %w", err) | ||
} | ||
if !shouldProcess { | ||
err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.WillNotProcess) | ||
err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.WillNotProcess, &request.Status) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
@@ -167,7 +167,7 @@ | |
if errors.Is(err, inventory.ErrUnsupportedChain) { | ||
// don't process request if chain is currently unsupported | ||
span.AddEvent("dropping unsupported chain") | ||
err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.WillNotProcess) | ||
err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.WillNotProcess, &request.Status) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
@@ -179,7 +179,7 @@ | |
|
||
// check if we have enough inventory to handle the request | ||
if committableBalance.Cmp(request.Transaction.DestAmount) < 0 { | ||
err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.NotEnoughInventory) | ||
err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.NotEnoughInventory, &request.Status) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
@@ -206,7 +206,7 @@ | |
} | ||
|
||
request.Status = reldb.CommittedPending | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
@@ -270,7 +270,7 @@ | |
} | ||
|
||
request.Status = reldb.CommittedConfirmed | ||
err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.CommittedConfirmed) | ||
err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.CommittedConfirmed, &request.Status) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
@@ -300,7 +300,7 @@ | |
span.AddEvent("relay successfully submitted") | ||
span.SetAttributes(attribute.Int("relay_nonce", int(nonce))) | ||
|
||
err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.RelayStarted) | ||
err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.RelayStarted, &request.Status) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update quote request status: %w", err) | ||
} | ||
|
@@ -332,7 +332,7 @@ | |
} | ||
|
||
// TODO: this can still get re-orged | ||
err = r.db.UpdateQuoteRequestStatus(ctx, req.TransactionId, reldb.RelayCompleted) | ||
err = r.db.UpdateQuoteRequestStatus(ctx, req.TransactionId, reldb.RelayCompleted, nil) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
@@ -453,14 +453,14 @@ | |
// Error Handlers Only from this point below. | ||
// | ||
// handleNotEnoughInventory handles the not enough inventory status. | ||
func (q *QuoteRequestHandler) handleNotEnoughInventory(ctx context.Context, _ trace.Span, request reldb.QuoteRequest) (err error) { | ||
func (q *QuoteRequestHandler) handleNotEnoughInventory(ctx context.Context, span trace.Span, request reldb.QuoteRequest) (err error) { | ||
Check failure on line 456 in services/rfq/relayer/service/handlers.go GitHub Actions / Lint (services/rfq)
|
||
committableBalance, err := q.Inventory.GetCommittableBalance(ctx, int(q.Dest.ChainID), request.Transaction.DestToken) | ||
if err != nil { | ||
return fmt.Errorf("could not get committable balance: %w", err) | ||
} | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage The status update logic in Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||
if err != nil { | ||
return fmt.Errorf("could not update request status: %w", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 0d573f2