Skip to content
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

Merged
merged 47 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
dadc913
use gql api
golangisfun123 Oct 9, 2024
dccd638
remove old test
golangisfun123 Oct 9, 2024
c1d21e4
[goreleaser]
golangisfun123 Oct 10, 2024
cd2187a
fix lint[goreleaser]
golangisfun123 Oct 10, 2024
8885dd5
[goreleaser]
golangisfun123 Oct 10, 2024
9dc780d
fix lint [goreleaser]
golangisfun123 Oct 10, 2024
2ef3bbf
explorer UI updated
Defi-Moses Oct 10, 2024
6cf146a
rfq-indexer update
Defi-Moses Oct 10, 2024
451a2d7
explorer backend update
Defi-Moses Oct 10, 2024
6c02517
[goreleaser] trigger explorer version bump
Defi-Moses Oct 10, 2024
a932e29
rfq indexer with the right contracts
Defi-Moses Oct 10, 2024
543fa15
fix logic
golangisfun123 Oct 10, 2024
50c4ecc
[goreleaser]
golangisfun123 Oct 10, 2024
a4cd9aa
fix error msg
golangisfun123 Oct 10, 2024
3e90715
[goreleaser] adding catch
Defi-Moses Oct 10, 2024
a40d79d
Merge branch 'master' of github.com:synapsecns/sanguine into opbot-mi…
golangisfun123 Oct 11, 2024
7c12249
remove function
golangisfun123 Oct 11, 2024
8724af9
Merge branch 'master' into feat/explorer-w
Defi-Moses Oct 12, 2024
9bd0038
response error fixes and wld decimals
Defi-Moses Oct 14, 2024
a7d5072
Merge branch 'master' into feat/explorer-w
Defi-Moses Oct 14, 2024
4f1f04c
origin tx hash
golangisfun123 Oct 14, 2024
1dbcd96
adding address
Defi-Moses Oct 15, 2024
6b4c78e
feat(rfq-indexer): add `request` column to `BridgeRequested` for refu…
golangisfun123 Oct 16, 2024
51d46c5
Merge branch 'master' of github.com:synapsecns/sanguine into opbot-mi…
golangisfun123 Oct 17, 2024
d0867c5
idr
golangisfun123 Oct 17, 2024
7679fde
Merge branch 'feat/explorer-w' of github.com:synapsecns/sanguine into…
golangisfun123 Oct 17, 2024
fce526b
merge
golangisfun123 Oct 17, 2024
146d28d
refund from rfq indexer
golangisfun123 Oct 17, 2024
111d862
add request field
golangisfun123 Oct 17, 2024
54b1391
yarn lcok
golangisfun123 Oct 17, 2024
0dd607d
Revert "yarn lcok"
golangisfun123 Oct 17, 2024
7835278
Revert "add request field"
golangisfun123 Oct 17, 2024
6c7c8f2
Merge branch 'master' of github.com:synapsecns/sanguine into opbot-mi…
golangisfun123 Oct 21, 2024
56bcab9
add new fiedl
golangisfun123 Oct 21, 2024
5e3f0e2
fix retry logic
golangisfun123 Oct 21, 2024
2e85f89
[goreleaser]
golangisfun123 Oct 21, 2024
2829c0f
Merge branch 'master' of github.com:synapsecns/sanguine into opbot-mi…
golangisfun123 Oct 28, 2024
88f9165
max attempt time [goreleaser]
golangisfun123 Oct 28, 2024
643c0dc
fix route [goreleaser]
golangisfun123 Oct 29, 2024
1193c9c
add linea explorer
golangisfun123 Oct 29, 2024
a4997e6
remove slash
golangisfun123 Oct 29, 2024
634144a
add worldchain explorer
golangisfun123 Oct 29, 2024
bc29e4d
fix lint
golangisfun123 Oct 29, 2024
72fa23b
remove 0x [goreleaser]
golangisfun123 Oct 30, 2024
69e03b2
Merge branch 'master' into opbot-migration
golangisfun123 Nov 6, 2024
f6c9020
revert quoter test
golangisfun123 Nov 6, 2024
1fbfbfa
fix lint
golangisfun123 Nov 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions contrib/opbot/botmd/botmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

"github.com/slack-io/slacker"
"github.com/synapsecns/sanguine/contrib/opbot/config"
"github.com/synapsecns/sanguine/contrib/opbot/internal"
"github.com/synapsecns/sanguine/contrib/opbot/signoz"
screenerClient "github.com/synapsecns/sanguine/contrib/screener-api/client"
"github.com/synapsecns/sanguine/core/dbcommon"
Expand All @@ -29,6 +30,7 @@
signozClient *signoz.Client
signozEnabled bool
rpcClient omnirpcClient.RPCClient
rfqClient internal.RFQClient
signer signer.Signer
submitter submitter.TransactionSubmitter
screener screenerClient.ScreenerClient
Expand All @@ -42,10 +44,11 @@
sugaredLogger := otelzap.New(experimentalLogger.MakeZapLogger()).Sugar()

bot := Bot{
handler: handler,
cfg: cfg,
server: server,
logger: experimentalLogger.MakeWrappedSugaredLogger(sugaredLogger),
handler: handler,
cfg: cfg,
server: server,
logger: experimentalLogger.MakeWrappedSugaredLogger(sugaredLogger),
rfqClient: internal.NewRFQClient(handler, cfg.RFQIndexerAPIURL, cfg.RelayerURLS),

Check warning on line 51 in contrib/opbot/botmd/botmd.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/botmd.go#L47-L51

Added lines #L47 - L51 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Issue Found: Missing RFQIndexerAPIURL in config.Config

The RFQIndexerAPIURL field is missing in the config.Config struct, which is required for the proper initialization of rfqClient.

  • File: contrib/opbot/config/config.go
🔗 Analysis chain

LGTM: Initialization of rfqClient and improved formatting

The initialization of the rfqClient in the NewBot function is correct and aligns with the new field added to the Bot struct. The reformatting of the struct initialization improves readability.

Please ensure that the RFQIndexerAPIURL and RelayerURLS fields have been added to the config.Config struct. Run the following script to verify:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the presence of new config fields

# Test: Check for RFQIndexerAPIURL and RelayerURLS in the config struct
rg --type go 'type\s+Config\s+struct' -A 20 | rg 'RFQIndexerAPIURL|RelayerURLS'

Length of output: 240

}

// you should be able to run opbot even without signoz.
Expand Down
213 changes: 65 additions & 148 deletions contrib/opbot/botmd/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
"regexp"
"sort"
"strings"
"sync"
"time"

"github.com/dustin/go-humanize"
Expand All @@ -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 {
Expand Down Expand Up @@ -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",

Check warning on line 160 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L160

Added line #L160 was not covered by tests
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()))

Check warning on line 170 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L167-L170

Added lines #L167 - L170 were not covered by tests
Comment on lines +160 to +170
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

- _, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error()))
+ _, err := ctx.Response().Reply("Error fetching quote request. Please try again later or contact support.")

Also, consider logging the detailed error for debugging purposes while keeping the user-facing message generic.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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()))
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) {
tx := stripLinks(ctx.Request().Param("tx"))
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("Error fetching quote request. Please try again later or contact support.")

Comment on lines +167 to +170
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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()))
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("Failed to fetch quote request. Please try again later.")
if err != nil {
log.Println(err)
}
return
}

if err != nil {
log.Println(err)
}
Expand All @@ -223,51 +176,46 @@

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))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential integer overflow when converting chain ID

The conversion of res.Bridge.OriginChainID to uint32 could lead to an integer overflow if the chain ID exceeds the maximum value of uint32 (4,294,967,295).

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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))),
},
}

Check warning on line 201 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L179-L201

Added lines #L179 - L201 were not covered by tests
Comment on lines +179 to +201
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

  • No tests exist for the Slack message block construction logic
  • Helper functions toExplorerSlackLink and toTXSlackLink are untested
  • Only a basic StripLinks utility function is tested

Key areas that need test coverage:

  • Message block construction with different bridge relay statuses
  • Link formatting for transaction hashes and explorer URLs
  • Edge cases for transaction hash formatting and chain IDs
🔗 Analysis chain

Add 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 executed

The 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
Added lines #L179 - L201 were not covered by tests


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))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential integer overflow when converting int to uint32

At line 210, res.Bridge.DestChainID is being converted from int to uint32 using uint32(res.Bridge.DestChainID). Similar to the issue at line 194, this could cause an integer overflow if res.Bridge.DestChainID exceeds the uint32 maximum value, leading to incorrect chain IDs.

Ensure that res.Bridge.DestChainID is within the uint32 range. If not, consider:

  • Switching to a larger unsigned integer type like uint64.
  • Adjusting the toTXSlackLink function to accept an int or int64 to safely handle larger chain ID values without risking overflow.
🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)

[failure] 210-210:
G115: integer overflow conversion int -> uint32 (gosec)

})

Check warning on line 213 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L203-L213

Added lines #L203 - L213 were not covered by tests
Comment on lines +203 to +213
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add unit tests for conditional handling of DestTxHash

Lines 203-213 introduce conditional logic to handle the presence or absence of DestTxHash based on the status. This conditional logic is not currently covered by unit tests. Consider adding tests to verify that the correct message is displayed for each possible status.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 203-213: contrib/opbot/botmd/commands.go#L203-L213
Added lines #L203 - L213 were not covered by tests

}

_, err := ctx.Response().ReplyBlocks(slackBlocks, slacker.WithUnfurlLinks(false))
slackBlocks = append(slackBlocks, slack.NewSectionBlock(nil, objects, nil))

_, err = ctx.Response().ReplyBlocks(slackBlocks, slacker.WithUnfurlLinks(false))

Check warning on line 218 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L216-L218

Added lines #L216 - L218 were not covered by tests
Comment on lines +216 to +218
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

[warning] 216-218: contrib/opbot/botmd/commands.go#L216-L218
Added lines #L216 - L218 were not covered by tests

Comment on lines +203 to +218
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

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:

  1. DRY Principle: The code for adding TextBlockObjects is repeated. Consider refactoring to a helper function:
func addTextBlockObject(objects []*slack.TextBlockObject, text string) []*slack.TextBlockObject {
    return append(objects, &slack.TextBlockObject{
        Type: slack.MarkdownType,
        Text: text,
    })
}
  1. Integer Overflow: There's another potential integer overflow when converting res.Bridge.DestChainID to uint32 on line 212. Apply the same safe casting solution as suggested for the OriginChainID.

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)
}
Expand All @@ -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)

Check warning on line 243 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L243

Added line #L243 was not covered by tests
if err != nil {
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err)
_, err := ctx.Response().Reply("error fetching quote request")
Expand All @@ -320,7 +259,7 @@
return
}

isScreened, err := b.screener.ScreenAddress(ctx.Context(), rawRequest.Sender)
isScreened, err := b.screener.ScreenAddress(ctx.Context(), rawRequest.Bridge.Sender)

Check warning on line 262 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L262

Added line #L262 was not covered by tests
if err != nil {
_, err := ctx.Response().Reply("error screening address")
if err != nil {
Expand All @@ -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))
if err != nil {
return nil, fmt.Errorf("error submitting refund: %w", err)
}
return tx, nil

Check warning on line 286 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L278-L286

Added lines #L278 - L286 were not covered by tests
})
if err != nil {
log.Printf("error submitting refund: %v\n", err)
return
Expand All @@ -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)

Check warning on line 297 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L297

Added line #L297 was not covered by tests
if err != nil || !status.HasTx() {
b.logger.Errorf(ctx, "error fetching quote request: %v", err)
return fmt.Errorf("error fetching quote request: %w", err)
Expand All @@ -364,20 +306,22 @@
)
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)
}

Check warning on line 312 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L309-L312

Added lines #L309 - L312 were not covered by tests
Comment on lines +309 to +312
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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
}

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 309-312: contrib/opbot/botmd/commands.go#L309-L312
Added lines #L309 - L312 were not covered by tests

return
}

_, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toExplorerSlackLink(status.TxHash().String())))
_, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toTXSlackLink(status.TxHash().String(), uint32(rawRequest.Bridge.OriginChainID))))

Check failure on line 316 in contrib/opbot/botmd/commands.go

View workflow job for this annotation

GitHub Actions / Lint (contrib/opbot)

G115: integer overflow conversion int -> uint32 (gosec)

Check warning on line 316 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L316

Added line #L316 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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))))

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)

[failure] 316-316:
G115: integer overflow conversion int -> uint32 (gosec)

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) {

Check warning on line 324 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L324

Added line #L324 was not covered by tests
client, err := rfqClient.NewUnauthenticatedClient(b.handler, b.cfg.RFQApiURL)
if err != nil {
return nil, fmt.Errorf("error creating rfq client: %w", err)
Expand All @@ -388,12 +332,12 @@
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, int(req.Bridge.OriginChainID))

Check failure on line 335 in contrib/opbot/botmd/commands.go

View workflow job for this annotation

GitHub Actions / Lint (contrib/opbot)

unnecessary conversion (unconvert)

Check warning on line 335 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L335

Added line #L335 was not covered by tests
if err != nil {
return nil, fmt.Errorf("error getting chain client: %w", err)
}

contractAddress, ok := contracts.Contracts[req.OriginChainID]
contractAddress, ok := contracts.Contracts[uint32(req.Bridge.OriginChainID)]

Check failure on line 340 in contrib/opbot/botmd/commands.go

View workflow job for this annotation

GitHub Actions / Lint (contrib/opbot)

G115: integer overflow conversion int -> uint32 (gosec)

Check warning on line 340 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L340

Added line #L340 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential integer overflow when accessing contract address

The conversion of req.Bridge.OriginChainID to uint32 could lead to an integer overflow if the chain ID exceeds the maximum value of uint32 (4,294,967,295).

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 safeUint32 is defined as:

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

[warning] 340-340: contrib/opbot/botmd/commands.go#L340
Added line #L340 was not covered by tests

🪛 GitHub Check: Lint (contrib/opbot)

[failure] 340-340:
G115: integer overflow conversion int -> uint32 (gosec)

if !ok {
return nil, errors.New("contract address not found")
}
Expand All @@ -405,24 +349,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") {

Check warning on line 355 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L355

Added line #L355 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure consistent hash prefix handling in toExplorerSlackLink

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)

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 356-356: contrib/opbot/botmd/commands.go#L356
Added line #L356 was not covered by tests

rfqHash = strings.ToLower(rfqHash[2:])
}

Comment on lines +357 to 360
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if strings.HasPrefix(rfqHash, "0X") {
rfqHash = strings.ToLower(rfqHash[2:])
}
if strings.HasPrefix(rfqHash, "0X") || strings.HasPrefix(rfqHash, "0x") {
rfqHash = strings.ToLower(rfqHash[2:])
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 355-355: contrib/opbot/botmd/commands.go#L355
Added line #L355 was not covered by tests

Expand All @@ -444,16 +374,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)
}
25 changes: 0 additions & 25 deletions contrib/opbot/botmd/commands_test.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
package botmd_test

import (
"context"
"testing"

"github.com/synapsecns/sanguine/contrib/opbot/botmd"
"github.com/synapsecns/sanguine/core/metrics"
omnirpcClient "github.com/synapsecns/sanguine/services/omnirpc/client"
"github.com/synapsecns/sanguine/services/rfq/relayer/relapi"
)

func TestStripLinks(t *testing.T) {
Expand All @@ -18,24 +14,3 @@ func TestStripLinks(t *testing.T) {
t.Errorf("StripLinks(%s) = %s; want %s", testLink, got, expected)
}
}

func TestTxAge(t *testing.T) {
notExpected := "unknown time ago" // should be a definite time

status := &relapi.GetQuoteRequestResponse{
OriginTxHash: "0x954264d120f5f3cf50edc39ebaf88ea9dc647d9d6843b7a120ed3677e23d7890",
OriginChainID: 421611,
}

ctx := context.Background()

client := omnirpcClient.NewOmnirpcClient("https://arb1.arbitrum.io/rpc", metrics.Get())
cc, err := client.GetChainClient(ctx, int(status.OriginChainID))
if err != nil {
t.Fatalf("GetChainClient() failed: %v", err)
}

if got := botmd.GetTxAge(context.Background(), cc, status); got == notExpected {
t.Errorf("TxAge(%s) = %s; want not %s", status.OriginTxHash, got, notExpected)
}
}
Loading
Loading