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

FE Release 2024-09-16 #3129

Merged
merged 16 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ linters:
- perfsprint
# malfunctions on embedded structs
- typecheck
# magic numbers
- mnd
Comment on lines +105 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

Enable the mnd linter to detect magic numbers.

The mnd linter is being added to the list of disabled linters, which contradicts the purpose of introducing it. To effectively detect and prevent the use of magic numbers in the codebase, the mnd linter should be enabled.

Remove the following lines to enable the mnd linter:

-    # magic numbers
-    - mnd
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
# magic numbers
- mnd

fast: false

issues:
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion agents/contracts/destination/destination.metadata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion agents/contracts/gasoracle/gasoracle.metadata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion agents/contracts/inbox/inbox.metadata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion agents/contracts/lightinbox/lightinbox.metadata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion agents/contracts/lightmanager/lightmanager.metadata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion agents/contracts/origin/origin.metadata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion agents/contracts/summit/summit.metadata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion agents/contracts/test/gasdata/gasdataharness.metadata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion agents/contracts/test/testclient/testclient.metadata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions contrib/opbot/botmd/botmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@
"github.com/synapsecns/sanguine/core/dbcommon"
"github.com/synapsecns/sanguine/core/metrics"
"github.com/synapsecns/sanguine/core/metrics/instrumentation/slackertrace"
experimentalLogger "github.com/synapsecns/sanguine/core/metrics/logger"
signerConfig "github.com/synapsecns/sanguine/ethergo/signer/config"
"github.com/synapsecns/sanguine/ethergo/signer/signer"
"github.com/synapsecns/sanguine/ethergo/submitter"
cctpSql "github.com/synapsecns/sanguine/services/cctp-relayer/db/sql"
omnirpcClient "github.com/synapsecns/sanguine/services/omnirpc/client"
"github.com/uptrace/opentelemetry-go-extra/otelzap"
"golang.org/x/sync/errgroup"
)

Expand All @@ -30,15 +32,20 @@
signer signer.Signer
submitter submitter.TransactionSubmitter
screener screenerClient.ScreenerClient
logger experimentalLogger.ExperimentalLogger
}

// NewBot creates a new bot server.
func NewBot(handler metrics.Handler, cfg config.Config) *Bot {
server := slacker.NewClient(cfg.SlackBotToken, cfg.SlackAppToken)

sugaredLogger := otelzap.New(experimentalLogger.MakeZapLogger()).Sugar()

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/botmd.go#L41-L43

Added lines #L41 - L43 were not covered by tests
bot := Bot{
handler: handler,
cfg: cfg,
server: server,
logger: experimentalLogger.MakeWrappedSugaredLogger(sugaredLogger),

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/botmd.go#L48

Added line #L48 was not covered by tests
}

// you should be able to run opbot even without signoz.
Expand Down
35 changes: 26 additions & 9 deletions contrib/opbot/botmd/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"log"
"math/big"
"regexp"
"sort"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -50,14 +51,16 @@
// TODO: add trace middleware.
func (b *Bot) traceCommand() *slacker.CommandDefinition {
return b.requiresSignoz(&slacker.CommandDefinition{
Command: "trace <tags>",
Command: "trace {tags} {order}",

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L54

Added line #L54 was not covered by tests
Description: "find a transaction in signoz",
Examples: []string{
"trace transaction_id:0x1234 serviceName:rfq",
"trace transaction_id:0x1234@serviceName:rfq",
"trace transaction_id:0x1234@serviceName:rfq a",
"trace transaction_id:0x1234@serviceName:rfq asc",

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L57-L59

Added lines #L57 - L59 were not covered by tests
},
Handler: func(ctx *slacker.CommandContext) {
tags := stripLinks(ctx.Request().Param("tags"))
splitTags := strings.Split(tags, " ")
splitTags := strings.Split(tags, "@")

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L63

Added line #L63 was not covered by tests
if len(splitTags) == 0 {
_, err := ctx.Response().Reply("please provide tags in a key:value format")
if err != nil {
Expand All @@ -82,6 +85,7 @@
// search for the transaction
res, err := b.signozClient.SearchTraces(ctx.Context(), signoz.Last3Hr, searchMap)
if err != nil {
b.logger.Errorf(ctx.Context(), "error searching for the transaction: %v", err)

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L88

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

Add a test case for the error scenario.

The error logging statement improves the visibility of potential failures. However, the static analysis tool indicates that this line is not covered by tests.

Please add a test case that simulates the error scenario to ensure the logging statement is executed as expected.

Do you want me to generate the test case or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

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

_, err := ctx.Response().Reply("error searching for the transaction")
if err != nil {
log.Println(err)
Expand All @@ -106,6 +110,14 @@
return
}

order := strings.ToLower(ctx.Request().Param("order"))
isAscending := order == "a" || order == "asc"
if isAscending {
sort.Slice(traceList, func(i, j int) bool {
return traceList[i].Timestamp.Before(traceList[j].Timestamp)
})

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L113-L118

Added lines #L113 - L118 were not covered by tests
}

Comment on lines +113 to +120
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure consistent sorting by setting a default sort order

Currently, if the 'order' parameter is not provided or doesn't match expected values, the traceList may not be sorted, leading to inconsistent results. It's recommended to define a default sort order (e.g., descending) and handle invalid inputs gracefully.

Apply this diff to set descending order as the default and handle invalid 'order' inputs:

 order := strings.ToLower(ctx.Request().Param("order"))
-isAscending := order == "a" || order == "asc"
-if isAscending {
-    sort.Slice(traceList, func(i, j int) bool {
-        return traceList[i].Timestamp.Before(traceList[j].Timestamp)
-    })
-}
+switch order {
+case "a", "asc":
+    sort.Slice(traceList, func(i, j int) bool {
+        return traceList[i].Timestamp.Before(traceList[j].Timestamp)
+    })
+case "", "d", "desc":
+    sort.Slice(traceList, func(i, j int) bool {
+        return traceList[i].Timestamp.After(traceList[j].Timestamp)
+    })
+default:
+    _, err := ctx.Response().Reply("Invalid 'order' parameter. Use 'asc' or 'desc'.")
+    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
order := strings.ToLower(ctx.Request().Param("order"))
isAscending := order == "a" || order == "asc"
if isAscending {
sort.Slice(traceList, func(i, j int) bool {
return traceList[i].Timestamp.Before(traceList[j].Timestamp)
})
}
order := strings.ToLower(ctx.Request().Param("order"))
switch order {
case "a", "asc":
sort.Slice(traceList, func(i, j int) bool {
return traceList[i].Timestamp.Before(traceList[j].Timestamp)
})
case "", "d", "desc":
sort.Slice(traceList, func(i, j int) bool {
return traceList[i].Timestamp.After(traceList[j].Timestamp)
})
default:
_, err := ctx.Response().Reply("Invalid 'order' parameter. Use 'asc' or 'desc'.")
if err != nil {
log.Println(err)
}
return
}

slackBlocks := []slack.Block{slack.NewHeaderBlock(slack.NewTextBlockObject(slack.PlainTextType, fmt.Sprintf("Traces for %s", tags), false, false))}

for _, results := range traceList {
Expand Down Expand Up @@ -290,6 +302,7 @@
}
}
if err != nil {
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err)

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L305

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

Add a test case for the error scenario.

The error logging statement improves the visibility of potential failures. However, the static analysis tool indicates that this line is not covered by tests.

Please add a test case that simulates the error scenario to ensure the logging statement is executed as expected.

Do you want me to generate the test case or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

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

_, err := ctx.Response().Reply("error fetching quote request")
if err != nil {
log.Println(err)
Expand Down Expand Up @@ -338,19 +351,23 @@
err = retry.WithBackoff(
ctx.Context(),
func(ctx context.Context) error {
txHash, err = relClient.GetTxHashByNonce(ctx, &relapi.GetTxByNonceRequest{
ChainID: rawRequest.OriginChainID,
Nonce: nonce,
})
txHash, err = relClient.GetTxHashByNonce(
ctx,
&relapi.GetTxByNonceRequest{
ChainID: rawRequest.OriginChainID,
Nonce: nonce,
})

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L354-L359

Added lines #L354 - L359 were not covered by tests
if err != nil {
b.logger.Errorf(ctx, "error fetching quote request: %v", err)

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L361

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

Add a test case for the error scenario.

The error logging statement improves the visibility of potential failures. However, the static analysis tool indicates that this line is not covered by tests.

Please add a test case that simulates the error scenario to ensure the logging statement is executed as expected.

Do you want me to generate the test case or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

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

return fmt.Errorf("error fetching quote request: %w", err)
}
return nil
},
retry.WithMaxAttempts(3),
retry.WithMaxAttemptTime(15*time.Second),
retry.WithMaxAttempts(5),
retry.WithMaxAttemptTime(30*time.Second),
)
if err != nil {
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err)

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L370

Added line #L370 was not covered by tests
Comment on lines +354 to +370
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Add test cases for rfqRefund retry mechanism

The review has revealed a lack of specific test cases for the rfqRefund function and its newly implemented retry mechanism. To ensure the reliability and correctness of this critical functionality, please consider the following actions:

  1. Create new test cases in contrib/opbot/botmd/commands_test.go specifically for the rfqRefund function.
  2. Implement tests that cover both successful scenarios and scenarios where retries are necessary.
  3. Use the structure of TestRetryWithBackoff in core/retry/retry_test.go as a reference for testing retry mechanisms.
  4. Ensure that the new tests verify the behavior with the specified MaxAttempts (5) and MaxAttemptTime (30 seconds).

Example test structure:

func TestRfqRefundRetryMechanism(t *testing.T) {
    // Test successful scenario
    t.Run("Success", func(t *testing.T) {
        // Implement test for successful rfqRefund execution
    })

    // Test retry scenario
    t.Run("RetrySuccess", func(t *testing.T) {
        // Implement test where rfqRefund succeeds after retries
    })

    // Test max retries exceeded
    t.Run("MaxRetriesExceeded", func(t *testing.T) {
        // Implement test where rfqRefund fails after max retries
    })

    // Test max attempt time exceeded
    t.Run("MaxAttemptTimeExceeded", func(t *testing.T) {
        // Implement test where rfqRefund fails due to timeout
    })
}

These tests will help verify the impact of the retry mechanism changes and ensure the robustness of the rfqRefund function.

Analysis chain

Add test cases for the retry mechanism changes and verify the impact.

The changes to the retry mechanism aim to provide a more robust handling of transient errors. However, the static analysis tool indicates that the added and modified lines are not covered by tests.

Please consider the following:

  1. Add test cases that simulate transient errors to ensure the retry mechanism behaves as expected.
  2. Verify the impact of the increased maximum attempts and attempt time on the function's behavior and performance.

Run the following script to analyze the impact of the retry mechanism changes:

Do you want me to generate the test cases or open a GitHub issue to track this task?

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Analyze the impact of the retry mechanism changes.

# Test: Search for the `retry.WithBackoff` usage. Expect: Only the updated usage.
rg --type go $'retry\\.WithBackoff' -A 5

# Test: Search for test cases that simulate transient errors. Expect: At least one test case.
rg --type go $'[Tt]est.*rfqRefund' -A 20 | rg $'err.*fetching quote request'

Length of output: 32450


Script:

#!/bin/bash
# Search for test files related to rfqRefund or commands.go
fd -e go test | xargs rg -i 'rfqrefund|commands\.go'

# Look for existing retry mechanism tests
rg --type go 'func Test.*Retry' -g '*_test.go'

# Check for any existing tests in the same package as commands.go
fd -e go | rg 'contrib/opbot/botmd/.*_test\.go'

Length of output: 292


Script:

#!/bin/bash
# Examine the contents of commands_test.go for rfqRefund related tests
rg -i 'func test.*rfqrefund' contrib/opbot/botmd/commands_test.go

# Check for any usage of retry mechanism in the test file
rg 'retry\.WithBackoff' contrib/opbot/botmd/commands_test.go

# Look at the structure of TestRetryWithBackoff for reference
rg -A 10 'func TestRetryWithBackoff' core/retry/retry_test.go

Length of output: 501

Tools
GitHub Check: codecov/patch

[warning] 343-348: contrib/opbot/botmd/commands.go#L343-L348
Added lines #L343 - L348 were not covered by tests


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


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

_, 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)
return
Expand Down
2 changes: 1 addition & 1 deletion contrib/opbot/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ require (
github.com/synapsecns/sanguine/services/cctp-relayer v0.0.0-00010101000000-000000000000
github.com/synapsecns/sanguine/services/omnirpc v0.0.0-00010101000000-000000000000
github.com/synapsecns/sanguine/services/rfq v0.0.0-00010101000000-000000000000
github.com/uptrace/opentelemetry-go-extra/otelzap v0.3.1
github.com/urfave/cli/v2 v2.27.2
golang.org/x/sync v0.8.0
gopkg.in/yaml.v2 v2.4.0
Expand Down Expand Up @@ -240,7 +241,6 @@ require (
github.com/uptrace/opentelemetry-go-extra/otelgorm v0.3.1 // indirect
github.com/uptrace/opentelemetry-go-extra/otelsql v0.3.1 // indirect
github.com/uptrace/opentelemetry-go-extra/otelutil v0.3.1 // indirect
github.com/uptrace/opentelemetry-go-extra/otelzap v0.3.1 // indirect
github.com/valyala/fastjson v1.6.4 // indirect
github.com/xanzy/ssh-agent v0.3.3 // indirect
github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect
Expand Down
2 changes: 1 addition & 1 deletion core/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ require (
go.uber.org/multierr v1.11.0
go.uber.org/zap v1.27.0
golang.org/x/sync v0.8.0
google.golang.org/grpc v1.64.0
gorm.io/driver/sqlite v1.5.6
gorm.io/gorm v1.25.10
k8s.io/apimachinery v0.29.3
Expand Down Expand Up @@ -189,7 +190,6 @@ require (
golang.org/x/tools v0.24.0 // indirect
google.golang.org/genproto/googleapis/api v0.0.0-20240701130421-f6361c86f094 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240701130421-f6361c86f094 // indirect
google.golang.org/grpc v1.64.0 // indirect
google.golang.org/protobuf v1.34.2 // indirect
gopkg.in/natefinch/lumberjack.v2 v2.0.0 // indirect
gopkg.in/warnings.v0 v0.1.2 // indirect
Expand Down
Loading
Loading