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

fix(opbot): fix opbot txage for l2s #2906

Merged
merged 3 commits into from
Jul 23, 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
24 changes: 10 additions & 14 deletions contrib/opbot/botmd/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"github.com/slack-io/slacker"
"github.com/synapsecns/sanguine/contrib/opbot/signoz"
"github.com/synapsecns/sanguine/ethergo/chaindata"
"github.com/synapsecns/sanguine/ethergo/client"
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"
Expand Down Expand Up @@ -209,9 +210,9 @@
var slackBlocks []slack.Block

for _, status := range statuses {
time, err := b.getTxAge(ctx, status.GetQuoteRequestStatusResponse)
client, err := b.rpcClient.GetChainClient(ctx.Context(), int(status.OriginChainID))

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#L213

Added line #L213 was not covered by tests
if err != nil {
log.Printf("error getting tx age: %v\n", err)
log.Printf("error getting chain client: %v\n", err)

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L215

Added line #L215 was not covered by tests
Comment on lines +213 to +215
Copy link

Choose a reason for hiding this comment

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

Logic: Error handling for GetChainClient should be more robust.

Comment on lines +213 to +215
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve error handling for GetChainClient.

The error handling for GetChainClient should be more robust. Consider handling cases where the client is nil to avoid potential nil pointer dereferences.

client, err := b.rpcClient.GetChainClient(ctx.Context(), int(status.OriginChainID))
if err != nil || client == nil {
	log.Printf("error getting chain client: %v\n", err)
	continue
}
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
client, err := b.rpcClient.GetChainClient(ctx.Context(), int(status.OriginChainID))
if err != nil {
log.Printf("error getting tx age: %v\n", err)
log.Printf("error getting chain client: %v\n", err)
client, err := b.rpcClient.GetChainClient(ctx.Context(), int(status.OriginChainID))
if err != nil || client == nil {
log.Printf("error getting chain client: %v\n", err)
continue

}

objects := []*slack.TextBlockObject{
Expand All @@ -233,7 +234,7 @@
},
{
Type: slack.MarkdownType,
Text: fmt.Sprintf("*Estimated Tx Age*: %s", humanize.Time(time)),
Text: fmt.Sprintf("*Estimated Tx Age*: %s", getTxAge(ctx.Context(), client, status.GetQuoteRequestStatusResponse)),

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L237

Added line #L237 was not covered by tests
},
}

Expand Down Expand Up @@ -347,23 +348,18 @@
return fastBridgeHandle, nil
}

func (b *Bot) getTxAge(ctx *slacker.CommandContext, status *relapi.GetQuoteRequestStatusResponse) (time.Time, error) {
func getTxAge(ctx context.Context, client client.EVM, res *relapi.GetQuoteRequestStatusResponse) string {
// TODO: add CreatedAt field to GetQuoteRequestStatusResponse so we don't need to make network calls?
client, err := b.rpcClient.GetChainClient(ctx.Context(), int(status.OriginChainID))
receipt, err := client.TransactionReceipt(ctx, common.HexToHash(res.OriginTxHash))
if err != nil {
return time.Time{}, fmt.Errorf("error getting chain client: %w", err)
return "unknown time ago"

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
}

receipt, err := client.TransactionReceipt(ctx.Context(), common.HexToHash(status.OriginTxHash))
if err != nil {
return time.Time{}, fmt.Errorf("error fetching transaction receipt: %w", err)
}
txBlock, err := client.BlockByHash(ctx.Context(), receipt.BlockHash)
txBlock, err := client.HeaderByHash(ctx, receipt.BlockHash)
if err != nil {
return time.Time{}, fmt.Errorf("error fetching block by hash: %w", err)
return "unknown time ago"

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#L359

Added line #L359 was not covered by tests
Comment on lines +357 to +359
Copy link

Choose a reason for hiding this comment

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

Logic: Error handling for HeaderByHash should be more robust.

Comment on lines +357 to +359
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve error handling for HeaderByHash.

The error handling for HeaderByHash should be more robust. Consider logging the error for better debugging.

txBlock, err := client.HeaderByHash(ctx, receipt.BlockHash)
if err != nil {
	log.Printf("error getting header by hash: %v\n", err)
	return "unknown time ago"
}
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
txBlock, err := client.HeaderByHash(ctx, receipt.BlockHash)
if err != nil {
return time.Time{}, fmt.Errorf("error fetching block by hash: %w", err)
return "unknown time ago"
txBlock, err := client.HeaderByHash(ctx, receipt.BlockHash)
if err != nil {
log.Printf("error getting header by hash: %v\n", err)
return "unknown time ago"
}

}

return time.Unix(int64(txBlock.Time()), 0), nil
return humanize.Time(time.Unix(int64(txBlock.Time), 0))
}

func toExplorerSlackLink(ogHash string) string {
Expand Down
28 changes: 27 additions & 1 deletion contrib/opbot/botmd/commands_test.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
package botmd_test

import (
"github.com/synapsecns/sanguine/contrib/opbot/botmd"
"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 @@ -13,3 +18,24 @@ 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.GetQuoteRequestStatusResponse{
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: 11 additions & 0 deletions contrib/opbot/botmd/export_test.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,16 @@
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.GetQuoteRequestStatusResponse) string {
return getTxAge(ctx, client, res)
}
Loading