-
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
[SLT-315] refactor(opbot): use gql api #3260
Changes from all commits
dadc913
dccd638
c1d21e4
cd2187a
8885dd5
9dc780d
2ef3bbf
6cf146a
451a2d7
6c02517
a932e29
543fa15
50c4ecc
a4cd9aa
3e90715
a40d79d
7c12249
8724af9
9bd0038
a7d5072
4f1f04c
1dbcd96
6b4c78e
51d46c5
d0867c5
7679fde
fce526b
146d28d
111d862
54b1391
0dd607d
7835278
6c7c8f2
56bcab9
5e3f0e2
2e85f89
2829c0f
88f9165
643c0dc
1193c9c
a4997e6
634144a
bc29e4d
72fa23b
69e03b2
f6c9020
1fbfbfa
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 | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,7 +11,6 @@ | |||||||||||||||||||||||||||
"regexp" | ||||||||||||||||||||||||||||
"sort" | ||||||||||||||||||||||||||||
"strings" | ||||||||||||||||||||||||||||
"sync" | ||||||||||||||||||||||||||||
"time" | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
"github.com/dustin/go-humanize" | ||||||||||||||||||||||||||||
|
@@ -21,14 +20,13 @@ | |||||||||||||||||||||||||||
"github.com/hako/durafmt" | ||||||||||||||||||||||||||||
"github.com/slack-go/slack" | ||||||||||||||||||||||||||||
"github.com/slack-io/slacker" | ||||||||||||||||||||||||||||
"github.com/synapsecns/sanguine/contrib/opbot/internal" | ||||||||||||||||||||||||||||
"github.com/synapsecns/sanguine/contrib/opbot/signoz" | ||||||||||||||||||||||||||||
"github.com/synapsecns/sanguine/core/retry" | ||||||||||||||||||||||||||||
"github.com/synapsecns/sanguine/ethergo/chaindata" | ||||||||||||||||||||||||||||
"github.com/synapsecns/sanguine/ethergo/client" | ||||||||||||||||||||||||||||
"github.com/synapsecns/sanguine/ethergo/submitter" | ||||||||||||||||||||||||||||
rfqClient "github.com/synapsecns/sanguine/services/rfq/api/client" | ||||||||||||||||||||||||||||
"github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge" | ||||||||||||||||||||||||||||
"github.com/synapsecns/sanguine/services/rfq/relayer/relapi" | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
func (b *Bot) requiresSignoz(definition *slacker.CommandDefinition) *slacker.CommandDefinition { | ||||||||||||||||||||||||||||
|
@@ -159,62 +157,17 @@ | |||||||||||||||||||||||||||
func (b *Bot) rfqLookupCommand() *slacker.CommandDefinition { | ||||||||||||||||||||||||||||
return &slacker.CommandDefinition{ | ||||||||||||||||||||||||||||
Command: "rfq <tx>", | ||||||||||||||||||||||||||||
Description: "find a rfq transaction by either tx hash or txid on all configured relayers", | ||||||||||||||||||||||||||||
Description: "find a rfq transaction by either tx hash or txid from the rfq-indexer api", | ||||||||||||||||||||||||||||
Examples: []string{ | ||||||||||||||||||||||||||||
"rfq 0x30f96b45ba689c809f7e936c140609eb31c99b182bef54fccf49778716a7e1ca", | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
Handler: func(ctx *slacker.CommandContext) { | ||||||||||||||||||||||||||||
type Status struct { | ||||||||||||||||||||||||||||
relayer string | ||||||||||||||||||||||||||||
*relapi.GetQuoteRequestResponse | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
var statuses []Status | ||||||||||||||||||||||||||||
var sliceMux sync.Mutex | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
if len(b.cfg.RelayerURLS) == 0 { | ||||||||||||||||||||||||||||
_, err := ctx.Response().Reply("no relayer urls configured") | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
log.Println(err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
tx := stripLinks(ctx.Request().Param("tx")) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
var wg sync.WaitGroup | ||||||||||||||||||||||||||||
// 2 routines per relayer, one for tx hashh one for tx id | ||||||||||||||||||||||||||||
wg.Add(len(b.cfg.RelayerURLS) * 2) | ||||||||||||||||||||||||||||
for _, relayer := range b.cfg.RelayerURLS { | ||||||||||||||||||||||||||||
client := relapi.NewRelayerClient(b.handler, relayer) | ||||||||||||||||||||||||||||
go func() { | ||||||||||||||||||||||||||||
defer wg.Done() | ||||||||||||||||||||||||||||
res, err := client.GetQuoteRequestByTxHash(ctx.Context(), tx) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
log.Printf("error fetching quote request status by tx hash: %v\n", err) | ||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
sliceMux.Lock() | ||||||||||||||||||||||||||||
defer sliceMux.Unlock() | ||||||||||||||||||||||||||||
statuses = append(statuses, Status{relayer: relayer, GetQuoteRequestResponse: res}) | ||||||||||||||||||||||||||||
}() | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
go func() { | ||||||||||||||||||||||||||||
defer wg.Done() | ||||||||||||||||||||||||||||
res, err := client.GetQuoteRequestByTXID(ctx.Context(), tx) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
log.Printf("error fetching quote request status by tx id: %v\n", err) | ||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
sliceMux.Lock() | ||||||||||||||||||||||||||||
defer sliceMux.Unlock() | ||||||||||||||||||||||||||||
statuses = append(statuses, Status{relayer: relayer, GetQuoteRequestResponse: res}) | ||||||||||||||||||||||||||||
}() | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
wg.Wait() | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
if len(statuses) == 0 { | ||||||||||||||||||||||||||||
_, err := ctx.Response().Reply("no quote request found") | ||||||||||||||||||||||||||||
res, status, err := b.rfqClient.GetRFQ(ctx.Context(), tx) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | ||||||||||||||||||||||||||||
_, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error())) | ||||||||||||||||||||||||||||
Comment on lines
+167
to
+170
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. Improve error handling to avoid exposing sensitive information. The current implementation directly exposes the error message to users, which could potentially leak sensitive information. Apply this diff to improve error handling: if err != nil {
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err)
- _, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error()))
+ _, err := ctx.Response().Reply("Failed to fetch quote request. Please try again later.")
if err != nil {
log.Println(err)
}
return
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
log.Println(err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
@@ -223,59 +176,54 @@ | |||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
var slackBlocks []slack.Block | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
for _, status := range statuses { | ||||||||||||||||||||||||||||
client, err := b.rpcClient.GetChainClient(ctx.Context(), int(status.OriginChainID)) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
log.Printf("error getting chain client: %v\n", err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
objects := []*slack.TextBlockObject{ | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*Relayer*: %s", status.relayer), | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*Status*: %s", status.Status), | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*TxID*: %s", toExplorerSlackLink(status.TxID)), | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(status.OriginTxHash, status.OriginChainID)), | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*Estimated Tx Age*: %s", getTxAge(ctx.Context(), client, status.GetQuoteRequestResponse)), | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
if status.DestTxHash == (common.Hash{}).String() { | ||||||||||||||||||||||||||||
objects = append(objects, &slack.TextBlockObject{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: "*DestTxHash*: not available", | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||
objects = append(objects, &slack.TextBlockObject{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(status.DestTxHash, status.DestChainID)), | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
objects := []*slack.TextBlockObject{ | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*Relayer*: %s", res.BridgeRelay.Relayer), | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*Status*: %s", status), | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*TxID*: %s", toExplorerSlackLink(res.Bridge.TransactionID)), | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
//nolint: gosec | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, uint32(res.Bridge.OriginChainID))), | ||||||||||||||||||||||||||||
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. Potential integer overflow when converting chain ID The conversion of Consider implementing a safe conversion function: func safeUint32(v int) uint32 {
if v < 0 || v > math.MaxUint32 {
// Handle error or return a default value
return 0
}
return uint32(v)
} Then use it in the function: - Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, uint32(res.Bridge.OriginChainID))),
+ Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, safeUint32(res.Bridge.OriginChainID))),
Comment on lines
+194
to
+195
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. Implement safe chain ID conversion to prevent integer overflow. The direct conversion of chain IDs to uint32 could cause integer overflow if the values exceed 4,294,967,295. Add a safe conversion function and use it: +func safeChainIDToUint32(chainID int) uint32 {
+ if chainID < 0 || chainID > math.MaxUint32 {
+ // Log the error and return a default value
+ return 0
+ }
+ return uint32(chainID)
+}
// Use in both places:
-Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, uint32(res.Bridge.OriginChainID))),
+Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, safeChainIDToUint32(res.Bridge.OriginChainID))),
-Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, uint32(res.Bridge.DestChainID))),
+Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, safeChainIDToUint32(res.Bridge.DestChainID))), Also applies to: 209-212 |
||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*Estimated Tx Age*: %s", humanize.Time(time.Unix(res.BridgeRelay.BlockTimestamp, 0))), | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
Comment on lines
+179
to
+201
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. 💡 Codebase verification Add unit tests for Slack message construction and helper functions The codebase currently lacks test coverage for Slack message construction and its helper functions. Specifically:
Key areas that need test coverage:
🔗 Analysis chainAdd unit tests for Slack message construction The new Slack message construction logic lacks test coverage, which could lead to undetected issues with message formatting or content. Would you like me to help create unit tests for the Slack message construction logic? Also applies to: 203-218 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check for existing tests of Slack message construction
rg -l "Test.*SlackMessage" --type go
Length of output: 38 Script: #!/bin/bash
# Let's check for any existing test files in the same directory
fd ".*_test.go" contrib/opbot/botmd/
# Also check for any test files that might be testing Slack-related functionality
rg -l "slack\." --type go | grep "_test"
# Look for the actual Slack message construction implementation
rg -B 5 -A 5 "slack\.TextBlockObject" --type go
Length of output: 2650 Script: #!/bin/bash
# Let's examine the existing test files in the botmd directory to see if they contain any Slack-related tests
cat contrib/opbot/botmd/commands_test.go
cat contrib/opbot/botmd/export_test.go
# Also check for any helper functions that might be used in Slack message construction
rg -B 2 -A 2 "toTXSlackLink|toExplorerSlackLink" --type go
Length of output: 2572 🧰 Tools🪛 GitHub Check: codecov/patch[warning] 179-201: contrib/opbot/botmd/commands.go#L179-L201 |
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
slackBlocks = append(slackBlocks, slack.NewSectionBlock(nil, objects, nil)) | ||||||||||||||||||||||||||||
if status == "Requested" { | ||||||||||||||||||||||||||||
objects = append(objects, &slack.TextBlockObject{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: "*DestTxHash*: not available", | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||
//nolint: gosec | ||||||||||||||||||||||||||||
objects = append(objects, &slack.TextBlockObject{ | ||||||||||||||||||||||||||||
Type: slack.MarkdownType, | ||||||||||||||||||||||||||||
Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, uint32(res.Bridge.DestChainID))), | ||||||||||||||||||||||||||||
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. Potential integer overflow when converting At line 210, Ensure that
🧰 Tools🪛 GitHub Check: Lint (contrib/opbot)
|
||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
Comment on lines
+203
to
+213
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. Add unit tests for conditional handling of Lines 203-213 introduce conditional logic to handle the presence or absence of 🧰 Tools🪛 GitHub Check: codecov/patch
|
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
_, err := ctx.Response().ReplyBlocks(slackBlocks, slacker.WithUnfurlLinks(false)) | ||||||||||||||||||||||||||||
slackBlocks = append(slackBlocks, slack.NewSectionBlock(nil, objects, nil)) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
_, err = ctx.Response().ReplyBlocks(slackBlocks, slacker.WithUnfurlLinks(false)) | ||||||||||||||||||||||||||||
Comment on lines
+216
to
+218
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. Handle errors when sending Slack message blocks When sending Slack message blocks (lines 216-218), if an error occurs, it is logged but not communicated to the user. Consider notifying the user when message delivery fails to enhance error transparency and user experience. 🧰 Tools🪛 GitHub Check: codecov/patch
Comment on lines
+203
to
+218
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. 🛠️ Refactor suggestion Improve DestTxHash handling and address potential integer overflow The conditional logic for handling different RFQ statuses improves the user experience by providing relevant information. However, there are two areas for improvement:
func addTextBlockObject(objects []*slack.TextBlockObject, text string) []*slack.TextBlockObject {
return append(objects, &slack.TextBlockObject{
Type: slack.MarkdownType,
Text: text,
})
}
Apply these changes to address both issues: func addTextBlockObject(objects []*slack.TextBlockObject, text string) []*slack.TextBlockObject {
return append(objects, &slack.TextBlockObject{
Type: slack.MarkdownType,
Text: text,
})
}
// In the main function
if status == "Requested" {
- objects = append(objects, &slack.TextBlockObject{
- Type: slack.MarkdownType,
- Text: "*DestTxHash*: not available",
- })
+ objects = addTextBlockObject(objects, "*DestTxHash*: not available")
} else {
- objects = append(objects, &slack.TextBlockObject{
- Type: slack.MarkdownType,
- Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, uint32(res.Bridge.DestChainID))),
- })
+ objects = addTextBlockObject(objects, fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, safeCastToUint32(res.Bridge.DestChainID))))
} These changes will improve code reusability and prevent potential runtime errors due to integer overflow. |
||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
log.Println(err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// nolint: gocognit, cyclop. | ||||||||||||||||||||||||||||
// nolint: gocognit, cyclop, gosec. | ||||||||||||||||||||||||||||
func (b *Bot) rfqRefund() *slacker.CommandDefinition { | ||||||||||||||||||||||||||||
return &slacker.CommandDefinition{ | ||||||||||||||||||||||||||||
Command: "refund <tx>", | ||||||||||||||||||||||||||||
|
@@ -292,16 +240,7 @@ | |||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
var rawRequest *relapi.GetQuoteRequestResponse | ||||||||||||||||||||||||||||
var err error | ||||||||||||||||||||||||||||
var relClient relapi.RelayerClient | ||||||||||||||||||||||||||||
for _, relayer := range b.cfg.RelayerURLS { | ||||||||||||||||||||||||||||
relClient = relapi.NewRelayerClient(b.handler, relayer) | ||||||||||||||||||||||||||||
rawRequest, err = getQuoteRequest(ctx.Context(), relClient, tx) | ||||||||||||||||||||||||||||
if err == nil { | ||||||||||||||||||||||||||||
break | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
rawRequest, _, err := b.rfqClient.GetRFQ(ctx.Context(), tx) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | ||||||||||||||||||||||||||||
_, err := ctx.Response().Reply("error fetching quote request") | ||||||||||||||||||||||||||||
|
@@ -320,7 +259,7 @@ | |||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
isScreened, err := b.screener.ScreenAddress(ctx.Context(), rawRequest.Sender) | ||||||||||||||||||||||||||||
isScreened, err := b.screener.ScreenAddress(ctx.Context(), rawRequest.Bridge.Sender) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
_, err := ctx.Response().Reply("error screening address") | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
|
@@ -336,13 +275,16 @@ | |||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(rawRequest.OriginChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||||||||||||||||||||||||||||
tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
return nil, fmt.Errorf("error submitting refund: %w", err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
return tx, nil | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
nonce, err := b.submitter.SubmitTransaction( | ||||||||||||||||||||||||||||
ctx.Context(), | ||||||||||||||||||||||||||||
big.NewInt(int64(rawRequest.Bridge.OriginChainID)), | ||||||||||||||||||||||||||||
func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||||||||||||||||||||||||||||
tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.Bridge.Request[2:])) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
return nil, fmt.Errorf("error submitting refund: %w", err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
return tx, nil | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
Comment on lines
+278
to
+287
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. Add error handling for transaction submission The function returns early on error without notifying the user about the transaction submission failure. Apply this diff to improve error handling: if err != nil {
log.Printf("error submitting refund: %v\n", err)
+ _, replyErr := ctx.Response().Reply("Failed to submit refund transaction. Please try again later.")
+ if replyErr != nil {
+ log.Println(replyErr)
+ }
return
}
🧰 Tools🪛 GitHub Check: codecov/patch[warning] 278-286: contrib/opbot/botmd/commands.go#L278-L286 |
||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
log.Printf("error submitting refund: %v\n", err) | ||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
|
@@ -352,7 +294,7 @@ | |||||||||||||||||||||||||||
err = retry.WithBackoff( | ||||||||||||||||||||||||||||
ctx.Context(), | ||||||||||||||||||||||||||||
func(ctx context.Context) error { | ||||||||||||||||||||||||||||
status, err = b.submitter.GetSubmissionStatus(ctx, big.NewInt(int64(rawRequest.OriginChainID)), nonce) | ||||||||||||||||||||||||||||
status, err = b.submitter.GetSubmissionStatus(ctx, big.NewInt(int64(rawRequest.Bridge.OriginChainID)), nonce) | ||||||||||||||||||||||||||||
if err != nil || !status.HasTx() { | ||||||||||||||||||||||||||||
b.logger.Errorf(ctx, "error fetching quote request: %v", err) | ||||||||||||||||||||||||||||
return fmt.Errorf("error fetching quote request: %w", err) | ||||||||||||||||||||||||||||
|
@@ -364,20 +306,23 @@ | |||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | ||||||||||||||||||||||||||||
_, err := ctx.Response().Reply(fmt.Sprintf("error fetching explorer link to refund, but nonce is %d", nonce)) | ||||||||||||||||||||||||||||
log.Printf("error fetching quote request: %v\n", err) | ||||||||||||||||||||||||||||
_, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
log.Println(err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
Comment on lines
+309
to
+312
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. Improve error handling after refund submission The current implementation sends a success message even when an error occurs during the status check. This could mislead users about the state of their refund. Consider modifying the error handling to provide more accurate feedback: if err != nil {
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err)
- _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce))
+ _, err := ctx.Response().Reply(fmt.Sprintf("Refund submitted with nonce %d, but there was an error confirming the status. Please check later.", nonce))
if err != nil {
log.Println(err)
}
return
}
🧰 Tools🪛 GitHub Check: codecov/patch
|
||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
_, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toExplorerSlackLink(status.TxHash().String()))) | ||||||||||||||||||||||||||||
//nolint: gosec | ||||||||||||||||||||||||||||
_, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toTXSlackLink(status.TxHash().String(), uint32(rawRequest.Bridge.OriginChainID)))) | ||||||||||||||||||||||||||||
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. Use safe chain ID conversion in toTXSlackLink call. Similar to previous occurrences, direct conversion of chain ID to uint32 could cause integer overflow. Apply this diff: -_, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toTXSlackLink(status.TxHash().String(), uint32(rawRequest.Bridge.OriginChainID))))
+_, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toTXSlackLink(status.TxHash().String(), safeChainIDToUint32(rawRequest.Bridge.OriginChainID))))
🧰 Tools🪛 GitHub Check: Lint (contrib/opbot)
|
||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
log.Println(err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
func (b *Bot) makeFastBridge(ctx context.Context, req *relapi.GetQuoteRequestResponse) (*fastbridge.FastBridge, error) { | ||||||||||||||||||||||||||||
func (b *Bot) makeFastBridge(ctx context.Context, req *internal.GetRFQByTxIDResponse) (*fastbridge.FastBridge, error) { | ||||||||||||||||||||||||||||
client, err := rfqClient.NewUnauthenticatedClient(b.handler, b.cfg.RFQApiURL) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
return nil, fmt.Errorf("error creating rfq client: %w", err) | ||||||||||||||||||||||||||||
|
@@ -388,12 +333,13 @@ | |||||||||||||||||||||||||||
return nil, fmt.Errorf("error fetching rfq contracts: %w", err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
chainClient, err := b.rpcClient.GetChainClient(ctx, int(req.OriginChainID)) | ||||||||||||||||||||||||||||
chainClient, err := b.rpcClient.GetChainClient(ctx, req.Bridge.OriginChainID) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
return nil, fmt.Errorf("error getting chain client: %w", err) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
contractAddress, ok := contracts.Contracts[req.OriginChainID] | ||||||||||||||||||||||||||||
//nolint: gosec | ||||||||||||||||||||||||||||
contractAddress, ok := contracts.Contracts[uint32(req.Bridge.OriginChainID)] | ||||||||||||||||||||||||||||
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. Potential integer overflow when accessing contract address The conversion of Consider using a safe conversion function similar to the one suggested earlier: - contractAddress, ok := contracts.Contracts[uint32(req.Bridge.OriginChainID)]
+ contractAddress, ok := contracts.Contracts[safeUint32(req.Bridge.OriginChainID)] Where func safeUint32(v int) uint32 {
if v < 0 || v > math.MaxUint32 {
// Handle error or return a default value
return 0
}
return uint32(v)
} 🧰 Tools🪛 GitHub Check: codecov/patch
🪛 GitHub Check: Lint (contrib/opbot)
|
||||||||||||||||||||||||||||
if !ok { | ||||||||||||||||||||||||||||
return nil, errors.New("contract address not found") | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
@@ -405,24 +351,10 @@ | |||||||||||||||||||||||||||
return fastBridgeHandle, nil | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
func getTxAge(ctx context.Context, client client.EVM, res *relapi.GetQuoteRequestResponse) string { | ||||||||||||||||||||||||||||
// TODO: add CreatedAt field to GetQuoteRequestStatusResponse so we don't need to make network calls? | ||||||||||||||||||||||||||||
receipt, err := client.TransactionReceipt(ctx, common.HexToHash(res.OriginTxHash)) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
return "unknown time ago" | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
txBlock, err := client.HeaderByHash(ctx, receipt.BlockHash) | ||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||
return "unknown time ago" | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
return humanize.Time(time.Unix(int64(txBlock.Time), 0)) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
func toExplorerSlackLink(ogHash string) string { | ||||||||||||||||||||||||||||
rfqHash := strings.ToUpper(ogHash) | ||||||||||||||||||||||||||||
// cut off 0x | ||||||||||||||||||||||||||||
if strings.HasPrefix(rfqHash, "0x") { | ||||||||||||||||||||||||||||
if strings.HasPrefix(rfqHash, "0X") { | ||||||||||||||||||||||||||||
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. Ensure consistent hash prefix handling in The current code checks for the "0X" prefix but does not handle the lowercase "0x" prefix. Consider updating the condition to handle both cases to prevent inconsistent behavior with different hash formats. Apply this diff to address the issue: if strings.HasPrefix(rfqHash, "0X") || strings.HasPrefix(rfqHash, "0x") {
rfqHash = rfqHash[2:]
}
rfqHash = strings.ToLower(rfqHash)
🧰 Tools🪛 GitHub Check: codecov/patch
|
||||||||||||||||||||||||||||
rfqHash = strings.ToLower(rfqHash[2:]) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
Comment on lines
+357
to
360
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. Improve hash prefix handling in toExplorerSlackLink The current implementation only handles the uppercase "0X" prefix. To ensure consistent behavior with different hash formats, consider updating the condition to handle both uppercase and lowercase prefixes. Apply this diff to address the issue: - if strings.HasPrefix(rfqHash, "0X") {
+ if strings.HasPrefix(rfqHash, "0X") || strings.HasPrefix(rfqHash, "0x") {
rfqHash = strings.ToLower(rfqHash[2:])
} This change will ensure that both "0X" and "0x" prefixes are handled consistently. 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: codecov/patch
|
||||||||||||||||||||||||||||
|
@@ -444,16 +376,3 @@ | |||||||||||||||||||||||||||
linkRegex := regexp.MustCompile(`<https?://[^|>]+\|([^>]+)>`) | ||||||||||||||||||||||||||||
return linkRegex.ReplaceAllString(input, "$1") | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
func getQuoteRequest(ctx context.Context, client relapi.RelayerClient, tx string) (qr *relapi.GetQuoteRequestResponse, err error) { | ||||||||||||||||||||||||||||
if qr, err = client.GetQuoteRequestByTxHash(ctx, tx); err == nil { | ||||||||||||||||||||||||||||
return qr, nil | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// look up quote request | ||||||||||||||||||||||||||||
if qr, err = client.GetQuoteRequestByTXID(ctx, tx); err == nil { | ||||||||||||||||||||||||||||
return qr, nil | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
return nil, fmt.Errorf("error fetching quote request: %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.
Improve error message handling in rfqLookupCommand
The refactored code simplifies the RFQ lookup process by using a dedicated client, which is a good improvement. However, there's a potential issue with error message construction:
On line 170, the error message directly includes
err.Error()
. This could lead to leaking sensitive information or presenting unclear messages to the user. Consider using a more generic error message or sanitizing the error before including it in the response.Suggestion:
Also, consider logging the detailed error for debugging purposes while keeping the user-facing message generic.
📝 Committable suggestion