-
Notifications
You must be signed in to change notification settings - Fork 33
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 all 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 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -89,7 +89,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 | ||||||||||||||
} | ||||||||||||||
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 +131,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.
Missing test coverage
The added lines for the
UpdateQuoteRequestStatus
function and theisValidStateTransition
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