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 11 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
182 changes: 48 additions & 134 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 @@ -24,7 +23,6 @@
"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"
Expand Down Expand Up @@ -164,57 +162,12 @@
"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.GetRFQByTxID(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
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 error handling in rfqLookupCommand

Lines 167-170 introduce new error handling when fetching the quote request using GetRFQByTxID. These lines are not covered by unit tests. Consider adding tests to ensure this error handling behaves as expected and to improve code coverage.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 167-170: contrib/opbot/botmd/commands.go#L167-L170
Added lines #L167 - L170 were not covered by tests

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.GetRFQByTxHash(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
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 fetching quote request in rfqRefund

Line 243 adds a call to GetRFQByTxHash in the rfqRefund command. This line is not covered by unit tests. Adding tests will help ensure that fetching the quote request and error handling work correctly.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 243-243: 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 Down Expand Up @@ -364,12 +303,14 @@
)
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 309 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L306 - L309 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(), rawRequest.OriginChainID)))

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L313

Added line #L313 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.

🛠️ Refactor suggestion

Handle errors when sending refund transaction link

Similar to the previous point, if there's an error when sending the refund transaction link, consider handling it appropriately to enhance user experience and error transparency.

🧰 Tools
🪛 GitHub Check: codecov/patch

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

if err != nil {
log.Println(err)
}
Expand Down Expand Up @@ -405,24 +346,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 352 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L352

Added line #L352 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 +371,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)
}
}
11 changes: 0 additions & 11 deletions contrib/opbot/botmd/export_test.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,5 @@
package botmd

import (
"context"

"github.com/synapsecns/sanguine/ethergo/client"
"github.com/synapsecns/sanguine/services/rfq/relayer/relapi"
)

func StripLinks(input string) string {
return stripLinks(input)
}

func GetTxAge(ctx context.Context, client client.EVM, res *relapi.GetQuoteRequestResponse) string {
return getTxAge(ctx, client, res)
}
2 changes: 2 additions & 0 deletions contrib/opbot/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ type Config struct {
ScreenerURL string `yaml:"screener_url"`
// Database is the database config.
Database DatabaseConfig `yaml:"database"`
// RFQIndexerAPIURL is the URL of the RFQ indexer API.
RFQIndexerAPIURL string `yaml:"rfq_indexer_api_url"`
Comment on lines +41 to +42
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

Consider enhancing the new RFQIndexerAPIURL field

The addition of the RFQIndexerAPIURL field is good, but there are a few suggestions to improve it:

  1. Add a comment explaining the purpose of this field, similar to other fields in the struct.
  2. Consider adding validation for this field in the Validate method if it's required for the application to function correctly.
  3. For consistency, you might want to rename RFQApiURL to RFQAPIUrl to match the naming convention of the new field.

Here's a suggested improvement for the field declaration:

+    // RFQIndexerAPIURL is the URL of the RFQ indexer API.
     RFQIndexerAPIURL string `yaml:"rfq_indexer_api_url"`

Consider updating the Validate method to include a check for this new field if necessary:

func (c *Config) Validate() error {
    // ... existing validations ...
    
    if c.RFQIndexerAPIURL == "" {
        return errors.New("rfq_indexer_api_url is required")
    }
    
    return nil
}

}

// DatabaseConfig represents the configuration for the database.
Expand Down
Loading
Loading