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

rfq: mimimal viable withdrawal api #2815

Merged
merged 12 commits into from
Jul 2, 2024
5 changes: 3 additions & 2 deletions contrib/opbot/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type Config struct {
// inject only with init container!
SignozPassword string `yaml:"signoz_password"`
// SignozBaseURL is the base url of the signoz instance.
SignozBaseURL string `yaml:"signoz_base_url"`
RelayerURLS []string `yaml:"rfq_relayer_urls"`
SignozBaseURL string `yaml:"signoz_base_url"`
// RelayerURLS is the list of RFQ relayer URLs.
RelayerURLS []string `yaml:"rfq_relayer_urls"`
}
6 changes: 6 additions & 0 deletions ethergo/submitter/submitter.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ type TransactionSubmitter interface {
SubmitTransaction(ctx context.Context, chainID *big.Int, call ContractCallType) (nonce uint64, err error)
// GetSubmissionStatus returns the status of a transaction and any metadata associated with it if it is complete.
GetSubmissionStatus(ctx context.Context, chainID *big.Int, nonce uint64) (status SubmissionStatus, err error)
// Address returns the address of the signer.
Address() common.Address
}

// txSubmitterImpl is the implementation of the transaction submitter.
Expand Down Expand Up @@ -665,4 +667,8 @@ func (t *txSubmitterImpl) getGasEstimate(ctx context.Context, chainClient client
return gasEstimate, nil
}

func (t *txSubmitterImpl) Address() common.Address {
return t.signer.Address()
}

var _ TransactionSubmitter = &txSubmitterImpl{}
2 changes: 2 additions & 0 deletions go.work.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1638,6 +1638,7 @@ github.com/Azure/go-autorest/logger v0.2.1/go.mod h1:T9E3cAhj2VqvPOtCYAvby9aBXkZ
github.com/Azure/go-autorest/tracing v0.6.0 h1:TYi4+3m5t6K48TGI9AUdb+IzbnSxvnvUMfuitfgcfuo=
github.com/Azure/go-autorest/tracing v0.6.0/go.mod h1:+vhtPC754Xsa23ID7GlGsrdKBpUA79WCAKPPZVC2DeU=
github.com/BurntSushi/toml v1.2.0/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ=
github.com/BurntSushi/toml v1.2.1/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ=
github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802 h1:1BDTz0u9nC3//pOCMdNH+CiXJVYJh5UQNCOBG7jbELc=
github.com/ClickHouse/clickhouse-go v1.5.4 h1:cKjXeYLNWVJIx2J1K6H2CqyRmfwVJVY1OV1coaaFcI0=
github.com/ClickHouse/clickhouse-go v1.5.4/go.mod h1:EaI/sW7Azgz9UATzd5ZdZHRUhHgv5+JMS9NSr2smCJI=
Expand Down Expand Up @@ -1993,6 +1994,7 @@ github.com/coreos/pkg v0.0.0-20180928190104-399ea9e2e55f/go.mod h1:E3G3o1h8I7cfc
github.com/cpuguy83/dockercfg v0.3.1 h1:/FpZ+JaygUR/lZP2NlFI2DVfrOEMAIKP5wWEJdoYe9E=
github.com/cpuguy83/dockercfg v0.3.1/go.mod h1:sugsbF4//dDlL/i+S+rtpIWp+5h0BHJHfjj5/jFyUJc=
github.com/cpuguy83/go-md2man v1.0.10 h1:BSKMNlYxDvnunlTymqtgONjNnaRV1sTpcovwwjF22jk=
github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o=
github.com/crackcomm/go-gitignore v0.0.0-20170627025303-887ab5e44cc3 h1:HVTnpeuvF6Owjd5mniCL8DEXo7uYXdQEmOP4FJbV5tg=
github.com/crackcomm/go-gitignore v0.0.0-20170627025303-887ab5e44cc3/go.mod h1:p1d6YEZWvFzEh4KLyvBcVSnrfNDDvK2zfK/4x2v/4pE=
github.com/crate-crypto/go-ipa v0.0.0-20220523130400-f11357ae11c7 h1:6IrxszG5G+O7zhtkWxq6+unVvnrm1fqV2Pe+T95DUzw=
Expand Down
2 changes: 1 addition & 1 deletion services/rfq/relayer/cmd/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
}

// commands
app.Commands = cli.Commands{runCommand}
app.Commands = cli.Commands{runCommand, withdrawCommand}

Check warning on line 26 in services/rfq/relayer/cmd/cmd.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/cmd/cmd.go#L26

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

Tip

Codebase Verification

No test coverage found for withdrawCommand.

It appears that the withdrawCommand logic is not covered by any tests. Please ensure that appropriate tests are added to verify the functionality of this command.

  • services/rfq/relayer/cmd/commands.go: Definition of withdrawCommand.
Analysis chain

Verify the integration of the withdrawCommand.

Ensure that the new withdrawCommand is correctly integrated into the CLI application.


Verify the definition and coverage of withdrawCommand.

Ensure that the withdrawCommand is correctly defined and that its logic is covered by tests.


Verify the test coverage of withdrawCommand.

Ensure that the withdrawCommand logic is covered by tests.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the integration of the withdrawCommand in the CLI application.

# Test: Search for the usage of withdrawCommand in the codebase. Expect: Proper integration in the CLI application.
rg --type go $'withdrawCommand'

Length of output: 193



Script:

#!/bin/bash
# Description: Verify the definition and test coverage of withdrawCommand.

# Step 1: Display the definition of withdrawCommand in commands.go
rg --type go --context 10 'var withdrawCommand' services/rfq/relayer/cmd/commands.go

# Step 2: Search for test files that might cover the withdrawCommand logic
fd --type f --name '*_test.go' | xargs rg 'withdrawCommand'

Length of output: 1169



Script:

#!/bin/bash
# Description: Verify the test coverage of withdrawCommand.

# Search for test files that might cover the withdrawCommand logic
fd --type f '_test.go' | xargs rg 'withdrawCommand'

Length of output: 49

Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure the new command withdrawCommand is covered by tests.

The addition of the new command should be tested to ensure it works as expected.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

shellCommand := commandline.GenerateShellCommand(app.Commands)
app.Commands = append(app.Commands, shellCommand)
app.Action = shellCommand.Action
Expand Down
78 changes: 78 additions & 0 deletions services/rfq/relayer/cmd/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
import (
"fmt"

"github.com/ethereum/go-ethereum/common"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure the new command withdrawCommand is covered by tests.

The addition of the new command should be tested to ensure it works as expected.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

"github.com/synapsecns/sanguine/core"
"github.com/synapsecns/sanguine/core/commandline"
"github.com/synapsecns/sanguine/core/metrics"
"github.com/synapsecns/sanguine/services/rfq/relayer/relapi"
"github.com/synapsecns/sanguine/services/rfq/relayer/relconfig"
"github.com/synapsecns/sanguine/services/rfq/relayer/service"
"github.com/urfave/cli/v2"
Expand Down Expand Up @@ -44,3 +46,79 @@
return nil
},
}

var relayerURLFlag = &cli.StringFlag{
Name: "relayer-url",
Usage: "relayer url",
}

var chainIDFlag = &cli.StringFlag{
Name: "chain-id",
Usage: "chain id",
}

var amountFlag = &cli.StringFlag{
Name: "amount",
Usage: "amount",
}

var tokenAddressFlag = &cli.StringFlag{
Name: "token-address",
Usage: "token address",
}

var toFlag = &cli.StringFlag{
Name: "to",
Usage: "to",
}

// runCommand runs the rfq relayer.
var withdrawCommand = &cli.Command{
Name: "withdraw",
Description: "run the withdrawal tool",
Flags: []cli.Flag{relayerURLFlag, chainIDFlag, amountFlag, tokenAddressFlag, toFlag, &commandline.LogLevel},
Action: func(c *cli.Context) (err error) {
if c.String(relayerURLFlag.Name) == "" {
return fmt.Errorf("relayer URL is required")
}

Check warning on line 83 in services/rfq/relayer/cmd/commands.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/cmd/commands.go#L80-L83

Added lines #L80 - L83 were not covered by tests
Comment on lines +81 to +83
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for relayerURLFlag validation.

Ensure that this validation is tested for various scenarios, such as missing or invalid URLs.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


client := relapi.NewRelayerClient(metrics.Get(), c.String(relayerURLFlag.Name))
if err != nil {
return fmt.Errorf("could not create relayer: %w", err)
}

Check warning on line 88 in services/rfq/relayer/cmd/commands.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/cmd/commands.go#L85-L88

Added lines #L85 - L88 were not covered by tests

chainID := c.Uint(chainIDFlag.Name)
if chainID == 0 {
return fmt.Errorf("valid chain ID is required")
}

Check warning on line 93 in services/rfq/relayer/cmd/commands.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/cmd/commands.go#L90-L93

Added lines #L90 - L93 were not covered by tests
Comment on lines +90 to +93
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for chainIDFlag validation.

Ensure that this validation is tested for various scenarios, such as missing or invalid chain IDs.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


amount := c.String(amountFlag.Name)
if amount == "" {
return fmt.Errorf("amount is required")
}

Check warning on line 98 in services/rfq/relayer/cmd/commands.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/cmd/commands.go#L95-L98

Added lines #L95 - L98 were not covered by tests
Comment on lines +95 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for amountFlag validation.

Ensure that this validation is tested for various scenarios, such as missing or invalid amounts.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


tokenAddress := c.String(tokenAddressFlag.Name)
if !common.IsHexAddress(tokenAddress) {
return fmt.Errorf("valid token address is required")
}

Check warning on line 103 in services/rfq/relayer/cmd/commands.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/cmd/commands.go#L100-L103

Added lines #L100 - L103 were not covered by tests
Comment on lines +100 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for tokenAddressFlag validation.

Ensure that this validation is tested for various scenarios, such as missing or invalid token addresses.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


to := c.String(toFlag.Name)
if !common.IsHexAddress(to) {
return fmt.Errorf("valid recipient address is required")
}

Check warning on line 108 in services/rfq/relayer/cmd/commands.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/cmd/commands.go#L105-L108

Added lines #L105 - L108 were not covered by tests
Comment on lines +105 to +108
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for toFlag validation.

Ensure that this validation is tested for various scenarios, such as missing or invalid recipient addresses.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


withdrawRequest := relapi.WithdrawRequest{
ChainID: uint32(chainID),
Amount: amount,
TokenAddress: common.HexToAddress(tokenAddress),
To: common.HexToAddress(to),
}

_, err = client.Withdraw(c.Context, &withdrawRequest)
if err != nil {
return fmt.Errorf("could not start relayer: %w", err)
Copy link

Choose a reason for hiding this comment

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

🪶 style: Error message should reflect the context of the operation, e.g., 'could not execute withdrawal'.

}

Check warning on line 120 in services/rfq/relayer/cmd/commands.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/cmd/commands.go#L110-L120

Added lines #L110 - L120 were not covered by tests
Comment on lines +117 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.

Add tests for the withdrawal process.

Ensure that the withdrawal process is tested for various scenarios, such as successful withdrawals, failed withdrawals, and edge cases.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


return nil

Check warning on line 122 in services/rfq/relayer/cmd/commands.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/cmd/commands.go#L122

Added line #L122 was not covered by tests
},
}
22 changes: 22 additions & 0 deletions services/rfq/relayer/relapi/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
GetQuoteRequestStatusByTxHash(ctx context.Context, hash string) (*GetQuoteRequestStatusResponse, error)
GetQuoteRequestStatusByTxID(ctx context.Context, hash string) (*GetQuoteRequestStatusResponse, error)
RetryTransaction(ctx context.Context, txhash string) (*GetTxRetryResponse, error)
Withdraw(ctx context.Context, req *WithdrawRequest) (*WithdrawResponse, error)
}

type relayerClient struct {
Expand Down Expand Up @@ -100,3 +101,24 @@

return &res, nil
}

// WithdrawResponse is the response for the withdraw request.
type WithdrawResponse struct {
Nonce uint64 `json:"nonce"`
}
trajan0x marked this conversation as resolved.
Show resolved Hide resolved

func (r *relayerClient) Withdraw(ctx context.Context, req *WithdrawRequest) (*WithdrawResponse, error) {
var res WithdrawResponse
resp, err := r.client.R().SetContext(ctx).
SetResult(&res).
SetBody(req).
Post(postWithdrawRoute)
if err != nil {
return nil, fmt.Errorf("failed to withdraw transaction: %w", err)
}

Check warning on line 118 in services/rfq/relayer/relapi/client.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/client.go#L117-L118

Added lines #L117 - L118 were not covered by tests
if resp.StatusCode() != http.StatusOK {
return nil, fmt.Errorf("unexpected status code: %d", resp.StatusCode())
}

Check warning on line 121 in services/rfq/relayer/relapi/client.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/client.go#L120-L121

Added lines #L120 - L121 were not covered by tests

return &res, nil
trajan0x marked this conversation as resolved.
Show resolved Hide resolved
}
11 changes: 11 additions & 0 deletions services/rfq/relayer/relapi/export_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package relapi

import (
"github.com/ethereum/go-ethereum/common"
"github.com/synapsecns/sanguine/services/rfq/relayer/relconfig"
)

// TokenIDExists checks if a token ID exists in the config.
func TokenIDExists(cfg relconfig.Config, tokenAddress common.Address, chainID int) bool {
return tokenIDExists(cfg, tokenAddress, chainID)
}
Comment on lines +8 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for TokenIDExists function.

Ensure that this function is tested for various scenarios, such as valid token addresses, invalid token addresses, and edge cases.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

172 changes: 165 additions & 7 deletions services/rfq/relayer/relapi/handler.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
package relapi

import (
"encoding/json"
"fmt"
"math/big"
"net/http"

"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/accounts/abi/bind"
"github.com/ethereum/go-ethereum/core/types"
"github.com/synapsecns/sanguine/ethergo/submitter"
"github.com/synapsecns/sanguine/services/rfq/contracts/ierc20"
"github.com/synapsecns/sanguine/services/rfq/relayer/relconfig"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/gin-gonic/gin"
Expand All @@ -13,15 +22,19 @@

// Handler is the REST API handler.
type Handler struct {
db reldb.Service
chains map[uint32]*chain.Chain
db reldb.Service
chains map[uint32]*chain.Chain
cfg relconfig.Config
submitter submitter.TransactionSubmitter
Comment on lines +27 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure the new fields in the Handler struct are properly initialized.

The addition of new fields should be tested to ensure they are correctly initialized and used.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

}

// NewHandler creates a new REST API handler.
func NewHandler(db reldb.Service, chains map[uint32]*chain.Chain) *Handler {
func NewHandler(db reldb.Service, chains map[uint32]*chain.Chain, cfg relconfig.Config, txSubmitter submitter.TransactionSubmitter) *Handler {
return &Handler{
db: db, // Store the database connection in the handler
chains: chains,
db: db, // Store the database connection in the handler
chains: chains,
cfg: cfg,
submitter: txSubmitter,
Comment on lines +32 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.

Tip

Codebase Verification

The NewHandler function is not covered by tests.

The search through the test files did not yield any results for the NewHandler function, indicating that it is not covered by unit tests. Please ensure that unit tests are added to cover various scenarios for this function.

  • Add unit tests for the NewHandler function.
Analysis chain

Ensure the NewHandler function is covered by tests.

The NewHandler function now includes additional parameters. Ensure that the function is covered by unit tests for various scenarios.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify if the NewHandler function is covered by tests.

# Test: Search for the usage of NewHandler in test files. Expect: Occurrences in test files.
rg --type go --glob '*_test.go' $'NewHandler'

Length of output: 44



Script:

#!/bin/bash
# Description: Verify if the NewHandler function is indirectly covered by tests.

# Test: Search for test functions that might indirectly call or cover the NewHandler function.
# First, list all test files
test_files=$(fd --type f --glob '*_test.go')

# Then search for the term 'NewHandler' within these test files
for file in $test_files; do
    echo "Searching in $file"
    rg 'NewHandler' $file
done

Length of output: 75358

Comment on lines +32 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.

Ensure the NewHandler function is covered by tests.

The addition of new parameters should be tested to ensure the function works as expected.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

}
}

Expand Down Expand Up @@ -107,14 +120,14 @@
}

chainID := quoteRequest.Transaction.DestChainId
chain, ok := h.chains[chainID]
chainHandler, ok := h.chains[chainID]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure the chainHandler variable is covered by tests.

The renaming of the variable should be tested to ensure it does not introduce any issues.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

if !ok {
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("No contract found for chain: %d", chainID)})
return
}

// `quoteRequest == nil` case should be handled by the db query above
nonce, gasAmount, err := chain.SubmitRelay(c, *quoteRequest)
nonce, gasAmount, err := chainHandler.SubmitRelay(c, *quoteRequest)
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not submit relay: %s", err.Error())})
return
Expand All @@ -128,3 +141,148 @@
}
c.JSON(http.StatusOK, resp)
}

// Withdraw withdraws tokens from the relayer.
//
//nolint:cyclop
Copy link

Choose a reason for hiding this comment

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

🪶 style: Consider refactoring to reduce cyclomatic complexity.

func (h *Handler) Withdraw(c *gin.Context) {
var req WithdrawRequest
if err := c.ShouldBindJSON(&req); err != nil {
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
return
}

Check warning on line 153 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L151-L153

Added lines #L151 - L153 were not covered by tests
trajan0x marked this conversation as resolved.
Show resolved Hide resolved

// validate the token address
if !tokenIDExists(h.cfg, req.TokenAddress, int(req.ChainID)) {
c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("invalid token address %s for chain %d", req.TokenAddress.Hex(), req.ChainID)})
return
}

Check warning on line 159 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L157-L159

Added lines #L157 - L159 were not covered by tests
trajan0x marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +157 to +159
Copy link
Contributor

@coderabbitai coderabbitai bot Jun 29, 2024

Choose a reason for hiding this comment

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

Add tests for tokenIDExists validation.

The validation of the token address in the Withdraw function is not covered by tests. Ensure that this validation is tested for various scenarios.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 153-155: services/rfq/relayer/relapi/handler.go#L153-L155
Added lines #L153 - L155 were not covered by tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor

Choose a reason for hiding this comment

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

@trajan0x, here are the unit tests for the tokenIDExists validation:

package relapi_test

import (
	"testing"

	"github.com/ethereum/go-ethereum/common"
	"github.com/synapsecns/sanguine/services/rfq/relayer/relconfig"
	"github.com/synapsecns/sanguine/services/rfq/relayer/relapi"
	"github.com/stretchr/testify/assert"
)

func TestTokenIDExists(t *testing.T) {
	cfg := relconfig.Config{
		QuotableTokens: map[string]struct{}{
			"1:0xTokenAddress1": {},
			"2:0xTokenAddress2": {},
		},
	}

	tests := []struct {
		name         string
		tokenAddress common.Address
		chainID      int
		expected     bool
	}{
		{
			name:         "Token exists",
			tokenAddress: common.HexToAddress("0xTokenAddress1"),
			chainID:      1,
			expected:     true,
		},
		{
			name:         "Token does not exist",
			tokenAddress: common.HexToAddress("0xNonExistentToken"),
			chainID:      1,
			expected:     false,
		},
		{
			name:         "Multiple tokens, correct token",
			tokenAddress: common.HexToAddress("0xTokenAddress2"),
			chainID:      2,
			expected:     true,
		},
		{
			name:         "Multiple tokens, incorrect token",
			tokenAddress: common.HexToAddress("0xTokenAddress1"),
			chainID:      2,
			expected:     false,
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			result := relapi.TokenIDExists(cfg, tt.tokenAddress, tt.chainID)
			assert.Equal(t, tt.expected, result)
		})
	}
}

This code tests various scenarios to ensure that the tokenIDExists function works as expected. Let me know if you need any further modifications or additional tests.


Learnings added
Learnt from: trajan0x
PR: synapsecns/sanguine#2815
File: services/rfq/relayer/relapi/handler.go:154-156
Timestamp: 2024-06-29T05:11:50.785Z
Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

trajan0x marked this conversation as resolved.
Show resolved Hide resolved

// validate the withdrawal address
if !toAddressIsWhitelisted(h.cfg, req.To) {
c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("withdrawal address %s is not whitelisted", req.To.Hex())})
return
}

Check warning on line 165 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L163-L165

Added lines #L163 - L165 were not covered by tests
Comment on lines +162 to +165
Copy link
Contributor

@coderabbitai coderabbitai bot Jun 29, 2024

Choose a reason for hiding this comment

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

Add tests for toAddressIsWhitelisted validation.

The validation of the withdrawal address in the Withdraw function is not covered by tests. Ensure that this validation is tested for various scenarios.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 158-161: services/rfq/relayer/relapi/handler.go#L158-L161
Added lines #L158 - L161 were not covered by tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you generate?

Copy link
Contributor

Choose a reason for hiding this comment

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

@trajan0x, sure! Here are the unit tests for the toAddressIsWhitelisted validation in the Withdraw function:

package relapi_test

import (
	"bytes"
	"encoding/json"
	"net/http"
	"net/http/httptest"
	"testing"

	"github.com/ethereum/go-ethereum/common"
	"github.com/gin-gonic/gin"
	"github.com/stretchr/testify/assert"
	"github.com/synapsecns/sanguine/services/rfq/relayer/relapi"
	"github.com/synapsecns/sanguine/services/rfq/relayer/relconfig"
	"github.com/synapsecns/sanguine/services/rfq/relayer/reldb"
	"github.com/synapsecns/sanguine/ethergo/submitter"
)

func TestWithdrawAddressWhitelist(t *testing.T) {
	gin.SetMode(gin.TestMode)

	// Create a mock configuration with a whitelist
	cfg := relconfig.Config{
		WithdrawalWhitelist: []string{
			"0x1111111111111111111111111111111111111111",
			"0x2222222222222222222222222222222222222222",
		},
	}

	// Create a mock handler with the mock configuration
	handler := relapi.NewHandler(reldb.NewMockService(), nil, cfg, submitter.NewMockTransactionSubmitter())

	tests := []struct {
		name       string
		toAddress  common.Address
		statusCode int
	}{
		{
			name:       "Address is whitelisted",
			toAddress:  common.HexToAddress("0x1111111111111111111111111111111111111111"),
			statusCode: http.StatusOK,
		},
		{
			name:       "Address is not whitelisted",
			toAddress:  common.HexToAddress("0x3333333333333333333333333333333333333333"),
			statusCode: http.StatusBadRequest,
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			reqBody := relapi.WithdrawRequest{
				ChainID:      1,
				Amount:       "1000000000000000000", // 1 ETH in wei
				TokenAddress: common.HexToAddress("0x0000000000000000000000000000000000000000"),
				To:           tt.toAddress,
			}
			body, err := json.Marshal(reqBody)
			assert.NoError(t, err)

			req, err := http.NewRequest(http.MethodPost, "/withdraw", bytes.NewBuffer(body))
			assert.NoError(t, err)
			req.Header.Set("Content-Type", "application/json")

			rec := httptest.NewRecorder()
			c, _ := gin.CreateTestContext(rec)
			c.Request = req

			handler.Withdraw(c)

			assert.Equal(t, tt.statusCode, rec.Code)
		})
	}
}

This test suite covers the validation of the withdrawal address in the Withdraw function by testing both whitelisted and non-whitelisted addresses.


Learnings added
Learnt from: trajan0x
PR: synapsecns/sanguine#2815
File: services/rfq/relayer/relapi/handler.go:162-165
Timestamp: 2024-07-02T14:43:19.123Z
Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios, including valid and invalid addresses.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


Learnings used
Learnt from: trajan0x
PR: synapsecns/sanguine#2815
File: services/rfq/relayer/relapi/handler.go:154-156
Timestamp: 2024-06-29T05:11:50.952Z
Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios.

Learnt from: trajan0x
PR: synapsecns/sanguine#2815
File: services/rfq/relayer/relapi/handler.go:153-156
Timestamp: 2024-06-29T05:12:50.268Z
Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

running the whole api for this is crazy. export it

Copy link
Contributor

Choose a reason for hiding this comment

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

@trajan0x, I understand your concern. We can export the toAddressIsWhitelisted function to facilitate direct testing. Here are the proposed changes:

  1. Export the toAddressIsWhitelisted function:

    // Export the function by renaming it to `ToAddressIsWhitelisted`.
    func ToAddressIsWhitelisted(cfg relconfig.Config, to common.Address) bool {
        for _, addr := range cfg.WithdrawalWhitelist {
            if common.HexToAddress(addr) == to {
                return true;
            }
        }
        return false;
    }
  2. Update the Withdraw function to use the exported function:

    // Update the function call in the Withdraw method.
    if !ToAddressIsWhitelisted(h.cfg, req.To) {
        c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("withdrawal address %s is not whitelisted", req.To.Hex())})
        return;
    }
  3. Unit tests for the ToAddressIsWhitelisted function:

    package relapi_test
    
    import (
        "testing"
    
        "github.com/ethereum/go-ethereum/common"
        "github.com/stretchr/testify/assert"
        "github.com/synapsecns/sanguine/services/rfq/relayer/relapi"
        "github.com/synapsecns/sanguine/services/rfq/relayer/relconfig"
    )
    
    func TestToAddressIsWhitelisted(t *testing.T) {
        cfg := relconfig.Config{
            WithdrawalWhitelist: []string{
                "0x1111111111111111111111111111111111111111",
                "0x2222222222222222222222222222222222222222",
            },
        }
    
        tests := []struct {
            name      string
            toAddress common.Address
            expected  bool
        }{
            {
                name:      "Address is whitelisted",
                toAddress: common.HexToAddress("0x1111111111111111111111111111111111111111"),
                expected:  true,
            },
            {
                name:      "Address is not whitelisted",
                toAddress: common.HexToAddress("0x3333333333333333333333333333333333333333"),
                expected:  false,
            },
        }
    
        for _, tt := range tests {
            t.Run(tt.name, func(t *testing.T) {
                result := relapi.ToAddressIsWhitelisted(cfg, tt.toAddress)
                assert.Equal(t, tt.expected, result)
            })
        }
    }

This approach ensures that the toAddressIsWhitelisted function is tested directly without the need to run the entire API.


Learnings added
Learnt from: trajan0x
PR: synapsecns/sanguine#2815
File: services/rfq/relayer/relapi/handler.go:162-165
Timestamp: 2024-07-02T14:49:46.333Z
Learning: When adding new validation logic in the `Withdraw` function, ensure to export helper functions like `toAddressIsWhitelisted` for direct unit testing.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


Learnings used
Learnt from: trajan0x
PR: synapsecns/sanguine#2815
File: services/rfq/relayer/relapi/handler.go:154-156
Timestamp: 2024-06-29T05:11:50.952Z
Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios.

Learnt from: trajan0x
PR: synapsecns/sanguine#2815
File: services/rfq/relayer/relapi/handler.go:153-156
Timestamp: 2024-06-29T05:12:50.268Z
Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios.

Learnt from: trajan0x
PR: synapsecns/sanguine#2815
File: services/rfq/relayer/relapi/handler.go:162-165
Timestamp: 2024-07-02T14:43:19.311Z
Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios, including valid and invalid addresses.

Learnt from: trajan0x
PR: synapsecns/sanguine#2815
File: services/rfq/relayer/relapi/handler.go:280-282
Timestamp: 2024-07-02T14:43:41.835Z
Learning: When the `UnmarshalJSON` method is added or modified in the `WithdrawRequest` struct, ensure to cover it with unit tests to handle various scenarios, including valid and invalid JSON inputs.

trajan0x marked this conversation as resolved.
Show resolved Hide resolved
trajan0x marked this conversation as resolved.
Show resolved Hide resolved

var nonce uint64
var err error

value, ok := new(big.Int).SetString(req.Amount, 10)
if !ok {
c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("invalid amount %s", req.Amount)})
return
}

Check warning on line 174 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L172-L174

Added lines #L172 - L174 were not covered by tests
trajan0x marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +172 to +174
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for amount validation.

The validation of the amount in the Withdraw function is not covered by tests. Ensure that this validation is tested for various scenarios.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 168-170: services/rfq/relayer/relapi/handler.go#L168-L170
Added lines #L168 - L170 were not covered by tests

trajan0x marked this conversation as resolved.
Show resolved Hide resolved

//nolint: nestif
Copy link

Choose a reason for hiding this comment

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

🪶 style: Consider refactoring to reduce nesting.

if chain.IsGasToken(req.TokenAddress) {
nonce, err = h.submitter.SubmitTransaction(c, big.NewInt(int64(req.ChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) {
bc := bind.NewBoundContract(req.To, abi.ABI{}, h.chains[req.ChainID].Client, h.chains[req.ChainID].Client, h.chains[req.ChainID].Client)
if transactor.GasPrice != nil {
transactor.Value = value
// nolint: wrapcheck
return bc.Transfer(transactor)
}
signer, err := transactor.Signer(h.submitter.Address(), tx)
if err != nil {
return nil, fmt.Errorf("could not get signer: %w", err)
}
return signer, nil

Check warning on line 189 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L185-L189

Added lines #L185 - L189 were not covered by tests
})
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not submit transaction: %s", err.Error())})
return
}

Check warning on line 194 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L192-L194

Added lines #L192 - L194 were not covered by tests
} else {
erc20Contract, err := ierc20.NewIERC20(req.TokenAddress, h.chains[req.ChainID].Client)
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not create erc20 contract: %s", err.Error())})
return
}

Check warning on line 200 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L198-L200

Added lines #L198 - L200 were not covered by tests
Comment on lines +192 to +200
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for gas token withdrawal.

The gas token withdrawal logic in the Withdraw function is not covered by tests. Ensure that this logic is tested for various scenarios.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 184-192: services/rfq/relayer/relapi/handler.go#L184-L192
Added lines #L184 - L192 were not covered by tests


nonce, err = h.submitter.SubmitTransaction(c, big.NewInt(int64(req.ChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) {
// nolint: wrapcheck
return erc20Contract.Transfer(transactor, req.To, value)
trajan0x marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

🪶 style: Consider adding error handling for the erc20Contract.Transfer call.

Comment on lines +202 to +204
Copy link

Choose a reason for hiding this comment

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

🪶 style: Consider adding error handling for the erc20Contract.Transfer call.

})
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not submit transaction: %s", err.Error())})
return
}

Check warning on line 209 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L207-L209

Added lines #L207 - L209 were not covered by tests
Comment on lines +202 to +209
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for ERC20 token withdrawal.

The ERC20 token withdrawal logic in the Withdraw function is not covered by tests. Ensure that this logic is tested for various scenarios.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 194-194: services/rfq/relayer/relapi/handler.go#L194
Added line #L194 was not covered by tests


[warning] 197-201: services/rfq/relayer/relapi/handler.go#L197-L201
Added lines #L197 - L201 were not covered by tests

Comment on lines +196 to +209
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for ERC20 token withdrawal.

The ERC20 token withdrawal logic in the Withdraw function is not covered by tests. Ensure that this logic is tested for various scenarios.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 191-193: services/rfq/relayer/relapi/handler.go#L191-L193
Added lines #L191 - L193 were not covered by tests


[warning] 200-202: services/rfq/relayer/relapi/handler.go#L200-L202
Added lines #L200 - L202 were not covered by tests

}

c.JSON(http.StatusOK, gin.H{"nonce": nonce})
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for successful withdrawal response.

The successful withdrawal response in the Withdraw function is not covered by tests. Ensure that this response is tested.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

}
Comment on lines +148 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.

Add tests for the Withdraw function.

Ensure that this function is tested for various scenarios, such as valid requests, invalid requests, and edge cases.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


// tokenIDExists checks if a token ID exists in the config.
// note: this method assumes that SanitizeTokenID is a method of relconfig.Config
func tokenIDExists(cfg relconfig.Config, tokenAddress common.Address, chainID int) bool {
for quotableToken := range cfg.QuotableTokens {
prospectiveChainID, prospectiveAddress, err := relconfig.DecodeTokenID(quotableToken)
if err != nil {
continue

Check warning on line 221 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L221

Added line #L221 was not covered by tests
Comment on lines +217 to +221
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for tokenIDExists function.

The tokenIDExists function is not covered by tests. Ensure that this function is tested for various scenarios.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 213-213: services/rfq/relayer/relapi/handler.go#L213
Added line #L213 was not covered by tests

}

if prospectiveChainID == chainID && prospectiveAddress == tokenAddress {
return true
}
}

return false
}
Comment on lines +215 to +230
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for tokenIDExists function.

Ensure that this function is tested for various scenarios, such as valid token addresses, invalid token addresses, and edge cases.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


func toAddressIsWhitelisted(cfg relconfig.Config, to common.Address) bool {
for _, addr := range cfg.WithdrawalWhitelist {
if common.HexToAddress(addr) == to {
return true
}
Comment on lines +232 to +236
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for toAddressIsWhitelisted function.

The toAddressIsWhitelisted function is not covered by tests. Ensure that this function is tested for various scenarios.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

}
return false

Check warning on line 238 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L238

Added line #L238 was not covered by tests
}
Comment on lines +232 to +239
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for toAddressIsWhitelisted function.

Ensure that this function is tested for various scenarios, such as valid addresses, invalid addresses, and edge cases.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


// WithdrawRequest is the request to withdraw tokens from the relayer.
type WithdrawRequest struct {
// ChainID is the chain ID of the chain to withdraw from.
ChainID uint32 `json:"chain_id"`
// Amount is the amount to withdraw, in wei.
Amount string `json:"amount"`
// TokenAddress is the address of the token to withdraw.
TokenAddress common.Address `json:"token_address"`
// To is the address to withdraw to.
To common.Address `json:"to"`
}

// MarshalJSON handles JSON marshaling for WithdrawRequest.
func (wr *WithdrawRequest) MarshalJSON() ([]byte, error) {
type Alias WithdrawRequest
// nolint: wrapcheck
return json.Marshal(&struct {
TokenAddress string `json:"token_address"`
To string `json:"to"`
*Alias
}{
TokenAddress: wr.TokenAddress.Hex(),
To: wr.To.Hex(),
Alias: (*Alias)(wr),
})
trajan0x marked this conversation as resolved.
Show resolved Hide resolved
}
Comment on lines +241 to +266
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for MarshalJSON method.

The MarshalJSON method in the WithdrawRequest struct is not covered by tests. Ensure that this method is tested for various scenarios.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?


// UnmarshalJSON has JSON unmarshalling for WithdrawRequest.
func (wr *WithdrawRequest) UnmarshalJSON(data []byte) error {
type Alias WithdrawRequest
aux := &struct {
TokenAddress string `json:"token_address"`
To string `json:"to"`
*Alias
}{
Alias: (*Alias)(wr),
}

if err := json.Unmarshal(data, aux); err != nil {
//nolint: wrapcheck
return err
}

Check warning on line 282 in services/rfq/relayer/relapi/handler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relapi/handler.go#L280-L282

Added lines #L280 - L282 were not covered by tests
trajan0x marked this conversation as resolved.
Show resolved Hide resolved

wr.TokenAddress = common.HexToAddress(aux.TokenAddress)
wr.To = common.HexToAddress(aux.To)

return nil
}
Comment on lines +268 to +288
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for UnmarshalJSON method.

Ensure that this method is tested for various scenarios, such as valid JSON inputs, invalid JSON inputs, and edge cases.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Loading
Loading