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

feat(opbot): add additional relay check [SLT-359] #3312

Merged
merged 11 commits into from
Nov 8, 2024
56 changes: 45 additions & 11 deletions contrib/opbot/botmd/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import (
"context"
"errors"
"fmt"
"log"
"math/big"
Expand All @@ -20,8 +19,8 @@
"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"
"github.com/synapsecns/sanguine/core/retry"
"github.com/synapsecns/sanguine/ethergo/chaindata"
"github.com/synapsecns/sanguine/ethergo/submitter"
Expand Down Expand Up @@ -250,7 +249,8 @@
return
}

fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest)
//nolint: gosec
fastBridgeContractOrigin, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.OriginChainID))

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L253

Added line #L253 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 bounds checking for chain ID conversions.

The conversions from int to uint32 for both origin and destination chain IDs could lead to integer overflow. Add validation to ensure the values are within uint32 range.

Apply this diff:

-fastBridgeContractOrigin, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.OriginChainID))
+if rawRequest.Bridge.OriginChainID < 0 || rawRequest.Bridge.OriginChainID > math.MaxUint32 {
+    return nil, fmt.Errorf("origin chain ID %d out of uint32 range", rawRequest.Bridge.OriginChainID)
+}
+fastBridgeContractOrigin, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.OriginChainID))

-fastBridgeContractDest, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.DestChainID))
+if rawRequest.Bridge.DestChainID < 0 || rawRequest.Bridge.DestChainID > math.MaxUint32 {
+    return nil, fmt.Errorf("destination chain ID %d out of uint32 range", rawRequest.Bridge.DestChainID)
+}
+fastBridgeContractDest, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.DestChainID))

Also applies to: 277-277

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

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

if err != nil {
_, err := ctx.Response().Reply(err.Error())
if err != nil {
Expand All @@ -275,11 +275,44 @@
return
}

//nolint:gosec
fastBridgeContractDest, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.DestChainID))
if err != nil {
_, err := ctx.Response().Reply(err.Error())
if err != nil {
log.Println(err)
}
return

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L279-L285

Added lines #L279 - L285 were not covered by tests
}
txBz, err := core.BytesToArray(common.Hex2Bytes(rawRequest.Bridge.TransactionID[2:]))
if err != nil {
_, err := ctx.Response().Reply("error converting tx id")
if err != nil {
log.Println(err)
}
return

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L287-L293

Added lines #L287 - L293 were not covered by tests
}
isRelayed, err := fastBridgeContractDest.BridgeRelays(nil, txBz)
if err != nil {
_, err := ctx.Response().Reply("error fetching bridge relays")
if err != nil {
log.Println(err)
}
return

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L295-L301

Added lines #L295 - L301 were not covered by tests
}
if isRelayed {
_, err := ctx.Response().Reply("transaction has already been relayed")
if err != nil {
log.Println(err)
}
return

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L303-L308

Added lines #L303 - L308 were not covered by tests
}
Comment on lines +280 to +309
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

Add test coverage for the relay check functionality.

The newly added relay check is a critical validation that prevents duplicate processing. However, this code path lacks test coverage.

Please add test cases covering:

  • Successful relay check
  • Error scenarios (invalid TxID, contract errors)
  • Edge cases (chain ID validation)

Would you like me to help create comprehensive test cases for these scenarios?

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 278-283: contrib/opbot/botmd/commands.go#L278-L283
Added lines #L278 - L283 were not covered by tests


[warning] 286-291: contrib/opbot/botmd/commands.go#L286-L291
Added lines #L286 - L291 were not covered by tests


[warning] 293-299: contrib/opbot/botmd/commands.go#L293-L299
Added lines #L293 - L299 were not covered by tests


[warning] 301-306: contrib/opbot/botmd/commands.go#L301-L306
Added lines #L301 - L306 were not covered by tests


nonce, err := b.submitter.SubmitTransaction(
ctx.Context(),
big.NewInt(int64(rawRequest.Bridge.OriginChainID)),
func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) {
tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.Bridge.Request[2:]))
tx, err = fastBridgeContractOrigin.Refund(transactor, common.Hex2Bytes(rawRequest.Bridge.Request[2:]))

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L315

Added line #L315 was not covered by tests
if err != nil {
return nil, fmt.Errorf("error submitting refund: %w", err)
}
Expand Down Expand Up @@ -322,7 +355,7 @@
}
}

func (b *Bot) makeFastBridge(ctx context.Context, req *internal.GetRFQByTxIDResponse) (*fastbridge.FastBridge, error) {
func (b *Bot) makeFastBridge(ctx context.Context, chainID uint32) (*fastbridge.FastBridge, error) {

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L358

Added line #L358 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 @@ -333,22 +366,23 @@
return nil, fmt.Errorf("error fetching rfq contracts: %w", err)
}

chainClient, err := b.rpcClient.GetChainClient(ctx, req.Bridge.OriginChainID)
chainClient, err := b.rpcClient.GetChainClient(ctx, int(chainID))

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L369

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

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L371

Added line #L371 was not covered by tests
}

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

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L374

Added line #L374 was not covered by tests
if !ok {
return nil, errors.New("contract address not found")
return nil, fmt.Errorf("no contract address for chain ID")

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L376

Added line #L376 was not covered by tests
}

fastBridgeHandle, err := fastbridge.NewFastBridge(common.HexToAddress(contractAddress), chainClient)
if err != nil {
return nil, fmt.Errorf("error creating fast bridge: %w", err)
return nil, fmt.Errorf("error creating fast bridge for chain ID %d: %w", chainID, err)

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

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L381

Added line #L381 was not covered by tests
}

return fastBridgeHandle, nil

}

func toExplorerSlackLink(ogHash string) string {
Expand Down
10 changes: 10 additions & 0 deletions core/bytes.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,13 @@

return string(formattedJSON), nil
}

// BytesToArray converts a slice to a 32 length byte array.
func BytesToArray(bz []byte) ([32]byte, error) {
golangisfun123 marked this conversation as resolved.
Show resolved Hide resolved
var bytes [32]byte
if len(bz) != 32 {
return bytes, fmt.Errorf("invalid length of bytes: %d", len(bz))
}
copy(bytes[:], bz)
return bytes, nil

Check warning on line 36 in core/bytes.go

View check run for this annotation

Codecov / codecov/patch

core/bytes.go#L30-L36

Added lines #L30 - L36 were not covered by tests
}
Comment on lines +30 to +37
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 BytesToArray function

The function lacks test coverage according to codecov report. As this is a core utility function, comprehensive testing is essential to ensure reliable behavior across different scenarios.

Would you like me to help generate unit tests covering:

  • Success case with valid 32-byte input
  • Error case with invalid length input
  • Edge cases with zero-filled arrays
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 30-36: core/bytes.go#L30-L36
Added lines #L30 - L36 were not covered by tests

Loading