-
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 19 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 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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,18 @@ | |||||||||||||
} | ||||||||||||||
|
||||||||||||||
// 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) { | ||||||||||||||
return nil | ||||||||||||||
} | ||||||||||||||
Comment on lines
+92
to
+102
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. ToolsGitHub Check: codecov/patch
Comment on lines
+100
to
+102
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
ToolsGitHub Check: codecov/patch
|
||||||||||||||
|
||||||||||||||
tx := s.DB().WithContext(ctx).Model(&RequestForQuote{}). | ||||||||||||||
Where(fmt.Sprintf("%s = ?", transactionIDFieldName), hexutil.Encode(id[:])). | ||||||||||||||
Update(statusFieldName, status) | ||||||||||||||
|
@@ -120,3 +135,10 @@ | |||||||||||||
} | ||||||||||||||
return nil | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
func isValidStateTransition(prevStatus, status reldb.QuoteRequestStatus) bool { | ||||||||||||||
if status == reldb.DeadlineExceeded || status == reldb.WillNotProcess { | ||||||||||||||
return true | ||||||||||||||
} | ||||||||||||||
return status >= prevStatus | ||||||||||||||
Comment on lines
+135
to
+139
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. Limited validation logic in 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. ToolsGitHub Check: codecov/patch
Missing test coverage The Do you want me to generate the unit testing code or open a GitHub issue to track this task? ToolsGitHub Check: codecov/patch
|
||||||||||||||
} |
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) | ||
} | ||
|
@@ -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. 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