rfq: mimimal viable withdrawal api #2815
Conversation
|
Warning Rate limit exceeded@trajan0x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 22 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe updates focus on adding functionality for handling and testing token withdrawals within the RFQ relayer service. This includes updates to the handler logic, configuration enhancements, test additions, and command-line interface updates to support withdrawal operations. Specific changes include new helper functions, structs, and tests interacting with blockchain backends, as well as refined configurations for token ID handling and whitelisting. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Handler
participant Config
participant Submitter
participant Blockchain
User->>CLI: Initiate withdrawal command
CLI->>Handler: Process withdrawal command
Handler->>Config: Validate token ID and address
Config->>Handler: Return validation status
Handler->>Submitter: Submit transaction
Submitter->>Blockchain: Interact with blockchain for withdrawal
Blockchain->>Submitter: Return transaction status
Submitter->>Handler: Provide transaction result
Handler->>User: Return withdrawal outcome
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
services/rfq/relayer/relapi/handler.go: AddedWithdrawfunction for token withdrawals, updatedHandlerstruct andNewHandlerfunction.services/rfq/relayer/relapi/server.go: Introduced withdrawal API endpoint, integratedsubmitter.TransactionSubmitter, updated server initialization.services/rfq/relayer/relconfig/config.go: Added fields toConfigfor enabling withdrawals and address whitelisting, introducedDecodeTokenIDfunction.services/rfq/relayer/relapi/handler_test.go: Added test forWithdrawRequeststruct.ethergo/submitter/submitter.go: UpdatedmeterNameconstant for correct OpenTelemetry metrics attribution.
5 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| // DecodeTokenID decodes a token ID into a chain ID and address. | ||
| func DecodeTokenID(id string) (chainID int, addr common.Address, err error) { | ||
| // defensive coding, first check if the token ID is valid | ||
| _, err = SanitizeTokenID(id) |
There was a problem hiding this comment.
🪶 style: Unnecessary call to SanitizeTokenID within DecodeTokenID. Consider removing it to avoid redundant validation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2815 +/- ##
===================================================
+ Coverage 25.64215% 25.73801% +0.09585%
===================================================
Files 766 766
Lines 54972 55148 +176
Branches 80 80
===================================================
+ Hits 14096 14194 +98
- Misses 39429 39494 +65
- Partials 1447 1460 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
5c4554a
|
| Status: | ⚡️ Build in progress... |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- ethergo/submitter/submitter.go (1 hunks)
- services/rfq/relayer/relapi/handler.go (4 hunks)
- services/rfq/relayer/relapi/handler_test.go (1 hunks)
- services/rfq/relayer/relapi/server.go (4 hunks)
- services/rfq/relayer/relconfig/config.go (2 hunks)
Files skipped from review due to trivial changes (1)
- ethergo/submitter/submitter.go
Additional context used
GitHub Check: codecov/patch
services/rfq/relayer/relapi/server.go
[warning] 123-124: services/rfq/relayer/relapi/server.go#L123-L124
Added lines #L123 - L124 were not covered by testsservices/rfq/relayer/relapi/handler.go
[warning] 144-149: services/rfq/relayer/relapi/handler.go#L144-L149
Added lines #L144 - L149 were not covered by tests
[warning] 152-155: services/rfq/relayer/relapi/handler.go#L152-L155
Added lines #L152 - L155 were not covered by tests
[warning] 158-161: services/rfq/relayer/relapi/handler.go#L158-L161
Added lines #L158 - L161 were not covered by tests
[warning] 163-170: services/rfq/relayer/relapi/handler.go#L163-L170
Added lines #L163 - L170 were not covered by tests
[warning] 172-191: services/rfq/relayer/relapi/handler.go#L172-L191
Added lines #L172 - L191 were not covered by tests
[warning] 193-200: services/rfq/relayer/relapi/handler.go#L193-L200
Added lines #L193 - L200 were not covered by tests
[warning] 203-203: services/rfq/relayer/relapi/handler.go#L203
Added line #L203 was not covered by tests
[warning] 208-212: services/rfq/relayer/relapi/handler.go#L208-L212
Added lines #L208 - L212 were not covered by tests
[warning] 215-217: services/rfq/relayer/relapi/handler.go#L215-L217
Added lines #L215 - L217 were not covered by tests
[warning] 220-220: services/rfq/relayer/relapi/handler.go#L220
Added line #L220 was not covered by tests
[warning] 223-227: services/rfq/relayer/relapi/handler.go#L223-L227
Added lines #L223 - L227 were not covered by tests
[warning] 229-229: services/rfq/relayer/relapi/handler.go#L229
Added line #L229 was not covered by tests
[warning] 245-256: services/rfq/relayer/relapi/handler.go#L245-L256
Added lines #L245 - L256 were not covered by tests
[warning] 271-273: services/rfq/relayer/relapi/handler.go#L271-L273
Added lines #L271 - L273 were not covered by testsservices/rfq/relayer/relconfig/config.go
[warning] 164-169: services/rfq/relayer/relconfig/config.go#L164-L169
Added lines #L164 - L169 were not covered by tests
[warning] 171-180: services/rfq/relayer/relconfig/config.go#L171-L180
Added lines #L171 - L180 were not covered by tests
Additional comments not posted (9)
services/rfq/relayer/relapi/handler_test.go (1)
1-34: Add more test cases forTestWithdrawRequestJSON.The current test only covers a single scenario. Consider adding more test cases to cover edge cases, such as:
- Missing fields in the JSON.
- Invalid JSON format.
- Different values for
ChainID,Amount,TokenAddress, andTo.func TestWithdrawRequestJSON(t *testing.T) { tests := []struct { name string original relapi.WithdrawRequest expectedJSON string expectedError bool }{ { name: "Valid JSON", original: relapi.WithdrawRequest{ ChainID: 1, Amount: "100", TokenAddress: common.HexToAddress("0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE"), To: common.HexToAddress("0xDeaDbeefdEAdbeefdEadbEEFdeadbeEFdEaDbeeF"), }, expectedJSON: `{"chain_id":1,"amount":"100","token_address":"0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee","to":"0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"}`, expectedError: false, }, { name: "Missing fields", original: relapi.WithdrawRequest{ ChainID: 1, }, expectedJSON: `{"chain_id":1,"amount":"","token_address":"","to":""}`, expectedError: false, }, // Add more test cases as needed... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { data, err := json.Marshal(tt.original) if tt.expectedError { assert.Error(t, err) return } assert.NoError(t, err) assert.JSONEq(t, tt.expectedJSON, string(data)) var unmarshalled relapi.WithdrawRequest err = json.Unmarshal(data, &unmarshalled) assert.NoError(t, err) assert.Equal(t, tt.original, unmarshalled) }) } }services/rfq/relayer/relapi/server.go (2)
28-33: Ensure thesubmitterfield is correctly initialized.The new
submitterfield in theRelayerAPIServerstruct should be properly initialized and used throughout the codebase.
90-94: Ensure all necessary fields are initialized inNewRelayerAPI.The
NewRelayerAPIfunction now initializes thesubmitterfield. Verify that all required fields are correctly initialized.Verification successful
All necessary fields are correctly initialized in
NewRelayerAPI.The
NewRelayerAPIfunction initializes all required fields:cfg,db,handler,chains, andsubmitter.
cfg: Initialized with the passedcfgparameter.db: Initialized with the passedstoreparameter.handler: Initialized with the passedhandlerparameter.chains: Initialized with the createdchainsmap.submitter: Initialized with the passedsubmitterparameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of all required fields in the `NewRelayerAPI` function. # Test: Search for the `NewRelayerAPI` function definition and check the initialization of fields. rg --type go 'func NewRelayerAPI'Length of output: 259
Script:
#!/bin/bash # Description: Extract the implementation of the `NewRelayerAPI` function from the relevant file. # Extract the full implementation of the NewRelayerAPI function. ast-grep --lang go --pattern 'func NewRelayerAPI($_) $_ { $$$ }' services/rfq/relayer/relapi/server.goLength of output: 104
Script:
#!/bin/bash # Description: Extract the implementation of the `NewRelayerAPI` function from the relevant file. # Extract the implementation of the NewRelayerAPI function including surrounding lines. rg --context 20 'func NewRelayerAPI' services/rfq/relayer/relapi/server.goLength of output: 1382
Script:
#!/bin/bash # Description: Extract the full implementation of the `NewRelayerAPI` function from the relevant file. # Extract the implementation of the NewRelayerAPI function including more surrounding lines. rg --context 40 'func NewRelayerAPI' services/rfq/relayer/relapi/server.goLength of output: 2913
Script:
#!/bin/bash # Description: Extract the full implementation of the `NewRelayerAPI` function from the relevant file. # Extract the implementation of the NewRelayerAPI function including more surrounding lines. rg --context 60 'func NewRelayerAPI' services/rfq/relayer/relapi/server.goLength of output: 3438
services/rfq/relayer/relapi/handler.go (5)
215-217: Add tests fortokenIDExistsfunction.The
tokenIDExistsfunction is not covered by tests. Ensure that this function is tested for various scenarios.Tools
GitHub Check: codecov/patch
[warning] 215-217: services/rfq/relayer/relapi/handler.go#L215-L217
Added lines #L215 - L217 were not covered by tests
220-220: Add tests fortokenIDExistsfunction.The
tokenIDExistsfunction is not covered by tests. Ensure that this function is tested for various scenarios.Tools
GitHub Check: codecov/patch
[warning] 220-220: services/rfq/relayer/relapi/handler.go#L220
Added line #L220 was not covered by tests
229-229: Add tests fortoAddressIsWhitelistedfunction.The
toAddressIsWhitelistedfunction is not covered by tests. Ensure that this function is tested for various scenarios.Tools
GitHub Check: codecov/patch
[warning] 229-229: services/rfq/relayer/relapi/handler.go#L229
Added line #L229 was not covered by tests
30-35: Ensure all necessary fields are initialized inNewHandler.The
NewHandlerfunction now initializes thecfgandsubmitterfields. Verify that all required fields are correctly initialized.Verification successful
All necessary fields are correctly initialized in
NewHandler.
- The
Handlerstruct contains the fieldsdb,chains,cfg, andsubmitter.- The
NewHandlerfunction initializes all these fields.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of all required fields in the `NewHandler` function. # Test: Search for the `NewHandler` function definition and check the initialization of fields. rg --type go 'func NewHandler'Length of output: 287
Script:
#!/bin/bash # Description: Verify that all required fields are initialized in the `NewHandler` function. # Step 1: Locate the definition of the `Handler` struct in the `services/rfq/relayer/relapi/handler.go` file. rg --type go 'type Handler struct' -A 20Length of output: 2751
23-26: Ensure thecfgandsubmitterfields are correctly initialized.The new
cfgandsubmitterfields in theHandlerstruct should be properly initialized and used throughout the codebase.Verification successful
Initialization and Usage of
cfgandsubmitterVerifiedThe
cfgandsubmitterfields in theHandlerstruct inservices/rfq/relayer/relapi/handler.goare correctly initialized in theNewHandlerfunction and used appropriately within the file.
Initialization in
NewHandlerfunction:func NewHandler(db reldb.Service, chains map[uint32]*chain.Chain, cfg relconfig.Config, txSubmitter submitter.TransactionSubmitter) *Handler { return &Handler{ db: db, chains: chains, cfg: cfg, submitter: txSubmitter, } }Usage in the file:
cfgis used in functions liketokenIDExistsandtoAddressIsWhitelisted.submitteris used inSubmitTransactioncalls.No further action is required as the fields are properly initialized and utilized.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and initialization of the `cfg` and `submitter` fields in the codebase. # Test: Search for the `cfg` and `submitter` fields usage. Expect: Proper initialization and usage. rg --type go 'cfg|submitter'Length of output: 87917
Script:
#!/bin/bash # Extract the NewHandler function and the usage of cfg and submitter in handler.go # Extract the NewHandler function ast-grep --lang go --pattern $'func NewHandler($_, $_, $_, $_) *Handler { $$$ }' services/rfq/relayer/relapi/handler.go # Extract the usage of cfg and submitter within handler.go rg --type go 'cfg|submitter' services/rfq/relayer/relapi/handler.goLength of output: 1811
services/rfq/relayer/relconfig/config.go (1)
54-57: Ensure proper documentation and validation for new configuration fields.The new fields
EnableAPIWithdrawalsandWithdrawalWhitelistshould be documented and validated to ensure they are used correctly.Consider adding comments to explain these fields and adding validation logic in the
Validatemethod.// EnableAPIWithdrawals enables withdrawals via the API. EnableAPIWithdrawals bool `yaml:"enable_api_withdrawals"` // WithdrawalWhitelist is a list of addresses that are allowed to withdraw. WithdrawalWhitelist []string `yaml:"withdrawal_whitelist"` // In the Validate method, add validation logic: if c.EnableAPIWithdrawals && len(c.WithdrawalWhitelist) == 0 { return fmt.Errorf("withdrawal whitelist cannot be empty if API withdrawals are enabled") }
| if r.cfg.EnableAPIWithdrawals { | ||
| engine.POST(postWithdrawRoute, h.Withdraw) | ||
| } |
There was a problem hiding this comment.
Add tests for the withdrawal route.
The POST /withdraw route is only added if EnableAPIWithdrawals is true. Ensure that this route is covered by tests.
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] 123-124: services/rfq/relayer/relapi/server.go#L123-L124
Added lines #L123 - L124 were not covered by tests
| 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 | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
running the whole api for this is crazy. export it
There was a problem hiding this comment.
@trajan0x, I understand your concern. We can export the toAddressIsWhitelisted function to facilitate direct testing. Here are the proposed changes:
-
Export the
toAddressIsWhitelistedfunction:// 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; }
-
Update the
Withdrawfunction 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; }
-
Unit tests for the
ToAddressIsWhitelistedfunction: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.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
Withdrawmethod toRelayerClientinterface in/services/rfq/relayer/relapi/client.go - Implemented
Withdrawmethod inHandlerstruct in/services/rfq/relayer/relapi/handler.go - Enhanced
Handlerstruct with configuration and transaction submitter fields in/services/rfq/relayer/relapi/handler.go - Added
TestEthWithdrawfunction to verify ETH withdrawal in/services/rfq/relayer/relapi/server_test.go - Updated test suite for withdrawal functionality in
/services/rfq/relayer/relapi/suite_test.go
5 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced new Withdraw API endpoint in
/services/rfq/relayer/relapi/handler.go - Updated
Handlerstruct with configuration and transaction submitter fields in/services/rfq/relayer/relapi/handler.go - Added JSON marshaling/unmarshaling for
WithdrawRequeststruct in/services/rfq/relayer/relapi/handler.go - Added test cases for ETH and ERC20 token withdrawals in
/services/rfq/relayer/relapi/server_test.go - Ensured withdrawal process updates balances correctly in
/services/rfq/relayer/relapi/server_test.go
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 17
Outside diff range and nitpick comments (1)
services/rfq/relayer/relapi/suite_test.go (1)
[!TIP]
Codebase VerificationPotential Race Condition Detected
Ensure proper synchronization when starting the transaction submitter in
services/rfq/relayer/relapi/suite_test.goto avoid the potential race condition. Consider using a wait group or a channel to ensurets.Startbegins only after necessary initializations.
services/rfq/relayer/relapi/suite_test.go: Lines around thets.Startcall.Analysis chain
Line range hint
100-157: LGTM!However, ensure to verify the potential race condition when starting the transaction submitter in a goroutine.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify potential race condition when starting the transaction submitter. # Test: Search for the transaction submitter start. Expect: No race conditions. rg --type go $'ts.Start'Length of output: 173
Script:
#!/bin/bash # Extract surrounding lines of ts.Start in suite_test.go rg --type go --context 10 'ts.Start' services/rfq/relayer/relapi/suite_test.go # Extract surrounding lines of ts.Start in submitter_test.go rg --type go --context 10 'ts.Start' ethergo/submitter/submitter_test.goLength of output: 1420
Tools
GitHub Check: Lint (services/rfq)
[failure] 182-182:
Using the variable on range scopechainIDin function literal (scopelint)
[failure] 189-189:
Using the variable on range scopechainIDin function literal (scopelint)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- ethergo/submitter/submitter.go (3 hunks)
- services/rfq/relayer/relapi/client.go (2 hunks)
- services/rfq/relayer/relapi/handler.go (4 hunks)
- services/rfq/relayer/relapi/server_test.go (2 hunks)
- services/rfq/relayer/relapi/suite_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- ethergo/submitter/submitter.go
Additional context used
GitHub Check: codecov/patch
services/rfq/relayer/relapi/client.go
[warning] 117-118: services/rfq/relayer/relapi/client.go#L117-L118
Added lines #L117 - L118 were not covered by tests
[warning] 120-121: services/rfq/relayer/relapi/client.go#L120-L121
Added lines #L120 - L121 were not covered by testsservices/rfq/relayer/relapi/handler.go
[warning] 147-149: services/rfq/relayer/relapi/handler.go#L147-L149
Added lines #L147 - L149 were not covered by tests
[warning] 153-155: services/rfq/relayer/relapi/handler.go#L153-L155
Added lines #L153 - L155 were not covered by tests
[warning] 159-161: services/rfq/relayer/relapi/handler.go#L159-L161
Added lines #L159 - L161 were not covered by tests
[warning] 168-170: services/rfq/relayer/relapi/handler.go#L168-L170
Added lines #L168 - L170 were not covered by tests
[warning] 184-192: services/rfq/relayer/relapi/handler.go#L184-L192
Added lines #L184 - L192 were not covered by tests
[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
[warning] 213-213: services/rfq/relayer/relapi/handler.go#L213
Added line #L213 was not covered by tests
[warning] 221-221: services/rfq/relayer/relapi/handler.go#L221
Added line #L221 was not covered by tests
[warning] 230-230: services/rfq/relayer/relapi/handler.go#L230
Added line #L230 was not covered by tests
[warning] 272-274: services/rfq/relayer/relapi/handler.go#L272-L274
Added lines #L272 - L274 were not covered by tests
GitHub Check: Lint (services/rfq)
services/rfq/relayer/relapi/server_test.go
[failure] 225-225:
ineffectual assignment to err (ineffassign)
[failure] 260-260:
ineffectual assignment to err (ineffassign)
[failure] 228-228:
error returned from interface method should be wrapped: sig: func (github.com/ethereum/go-ethereum.ChainStateReader).BalanceAt(ctx context.Context, account github.com/ethereum/go-ethereum/common.Address, blockNumber *math/big.Int) (*math/big.Int, error) (wrapcheck)
[failure] 263-263:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/testcontracts/mockerc20.MockERC20Caller).BalanceOf(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.CallOpts, account github.com/ethereum/go-ethereum/common.Address) (*math/big.Int, error) (wrapcheck)services/rfq/relayer/relapi/handler.go
[failure] 181-181:
error returned from external package is unwrapped: sig: field Signer github.com/ethereum/go-ethereum/accounts/abi/bind.SignerFn (wrapcheck)services/rfq/relayer/relapi/suite_test.go
[failure] 182-182:
Using the variable on range scopechainIDin function literal (scopelint)
[failure] 189-189:
Using the variable on range scopechainIDin function literal (scopelint)
Additional comments not posted (3)
services/rfq/relayer/relapi/suite_test.go (3)
Line range hint
41-57: LGTM!
58-59: LGTM!
159-161: LGTM!
| c.deployManager = testutil.NewDeployManager(c.T()) | ||
|
|
||
| // let's create 2 mock chains | ||
| chainIDs := []uint64{1, 42161} | ||
|
|
||
| c.testBackends = make(map[uint64]backends.SimulatedTestBackend) | ||
|
|
||
| testWallet, err := wallet.FromRandom() | ||
| c.Require().NoError(err) | ||
| c.testWallet = testWallet | ||
|
|
||
| g, _ := errgroup.WithContext(c.GetSuiteContext()) | ||
| for _, chainID := range chainIDs { | ||
| // Setup Anvil backend for the suite to have RPC support | ||
| // anvilOpts := anvil.NewAnvilOptionBuilder() | ||
| // anvilOpts.SetChainID(chainID) | ||
| // anvilOpts.SetBlockTime(1 * time.Second) | ||
| // backend := anvil.NewAnvilBackend(c.GetSuiteContext(), c.T(), anvilOpts) | ||
| backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID)) | ||
|
|
||
| // add the backend to the list of backends | ||
| c.testBackends[chainID] = backend | ||
| c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend) | ||
| g.Go(func() error { | ||
| backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID)) | ||
|
|
||
| backend.FundAccount(c.GetSuiteContext(), c.testWallet.Address(), *big.NewInt(params.Ether)) | ||
|
|
||
| // add the backend to the list of backends | ||
| c.testBackendMux.Lock() | ||
| defer c.testBackendMux.Unlock() | ||
| c.testBackends[chainID] = backend | ||
| c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend) | ||
|
|
||
| return nil | ||
| }) |
There was a problem hiding this comment.
Fix the scope issue with the chainID variable.
The variable chainID should be passed as an argument to the function literal to avoid scope issues.
- for _, chainID := range chainIDs {
+ for _, id := range chainIDs {
// Setup Anvil backend for the suite to have RPC support
- g.Go(func() error {
+ g.Go(func(chainID uint64) func() error {
+ return func() error {
backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID))
backend.FundAccount(c.GetSuiteContext(), c.testWallet.Address(), *big.NewInt(params.Ether))
// add the backend to the list of backends
c.testBackendMux.Lock()
defer c.testBackendMux.Unlock()
c.testBackends[chainID] = backend
c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend)
return nil
}
- })
+ }(id))
}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.
| c.deployManager = testutil.NewDeployManager(c.T()) | |
| // let's create 2 mock chains | |
| chainIDs := []uint64{1, 42161} | |
| c.testBackends = make(map[uint64]backends.SimulatedTestBackend) | |
| testWallet, err := wallet.FromRandom() | |
| c.Require().NoError(err) | |
| c.testWallet = testWallet | |
| g, _ := errgroup.WithContext(c.GetSuiteContext()) | |
| for _, chainID := range chainIDs { | |
| // Setup Anvil backend for the suite to have RPC support | |
| // anvilOpts := anvil.NewAnvilOptionBuilder() | |
| // anvilOpts.SetChainID(chainID) | |
| // anvilOpts.SetBlockTime(1 * time.Second) | |
| // backend := anvil.NewAnvilBackend(c.GetSuiteContext(), c.T(), anvilOpts) | |
| backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID)) | |
| // add the backend to the list of backends | |
| c.testBackends[chainID] = backend | |
| c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend) | |
| g.Go(func() error { | |
| backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID)) | |
| backend.FundAccount(c.GetSuiteContext(), c.testWallet.Address(), *big.NewInt(params.Ether)) | |
| // add the backend to the list of backends | |
| c.testBackendMux.Lock() | |
| defer c.testBackendMux.Unlock() | |
| c.testBackends[chainID] = backend | |
| c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend) | |
| return nil | |
| }) | |
| c.deployManager = testutil.NewDeployManager(c.T()) | |
| // let's create 2 mock chains | |
| chainIDs := []uint64{1, 42161} | |
| c.testBackends = make(map[uint64]backends.SimulatedTestBackend) | |
| testWallet, err := wallet.FromRandom() | |
| c.Require().NoError(err) | |
| c.testWallet = testWallet | |
| g, _ := errgroup.WithContext(c.GetSuiteContext()) | |
| for _, id := range chainIDs { | |
| // Setup Anvil backend for the suite to have RPC support | |
| g.Go(func(chainID uint64) func() error { | |
| return func() error { | |
| backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID)) | |
| backend.FundAccount(c.GetSuiteContext(), c.testWallet.Address(), *big.NewInt(params.Ether)) | |
| // add the backend to the list of backends | |
| c.testBackendMux.Lock() | |
| defer c.testBackendMux.Unlock() | |
| c.testBackends[chainID] = backend | |
| c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend) | |
| return nil | |
| } | |
| }(id)) | |
| } |
Tools
GitHub Check: Lint (services/rfq)
[failure] 182-182:
Using the variable on range scopechainIDin function literal (scopelint)
[failure] 189-189:
Using the variable on range scopechainIDin function literal (scopelint)
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| balance, err := backend.BalanceAt(ctx, testWithdrawalAddress, nil) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Wrap error returned from external package.
The error returned from the external package should be wrapped to provide more context.
- return err
+ return fmt.Errorf("failed to get balance: %w", err)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.
| return err | |
| return fmt.Errorf("failed to get balance: %w", err) |
Tools
GitHub Check: Lint (services/rfq)
[failure] 228-228:
error returned from interface method should be wrapped: sig: func (github.com/ethereum/go-ethereum.ChainStateReader).BalanceAt(ctx context.Context, account github.com/ethereum/go-ethereum/common.Address, blockNumber *math/big.Int) (*math/big.Int, error) (wrapcheck)
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Wrap error returned from external package.
The error returned from the external package should be wrapped to provide more context.
- return err
+ return fmt.Errorf("failed to get balance: %w", err)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.
| return err | |
| return fmt.Errorf("failed to get balance: %w", err) |
Tools
GitHub Check: Lint (services/rfq)
[failure] 263-263:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/testcontracts/mockerc20.MockERC20Caller).BalanceOf(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.CallOpts, account github.com/ethereum/go-ethereum/common.Address) (*math/big.Int, error) (wrapcheck)
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not submit transaction: %s", err.Error())}) | ||
| return | ||
| } | ||
| } 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 | ||
| } |
There was a problem hiding this comment.
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) | ||
| }) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not submit transaction: %s", err.Error())}) | ||
| return | ||
| } |
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
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
| func toAddressIsWhitelisted(cfg relconfig.Config, to common.Address) bool { | ||
| for _, addr := range cfg.WithdrawalWhitelist { | ||
| if common.HexToAddress(addr) == to { | ||
| return true | ||
| } |
There was a problem hiding this comment.
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?
| Value: value, | ||
| }) | ||
|
|
||
| return transactor.Signer(h.submitter.Address(), tx) |
There was a problem hiding this comment.
Wrap error returned from external package.
The error returned from the external package should be wrapped to provide more context.
- return transactor.Signer(h.submitter.Address(), tx)
+ return transactor.Signer(h.submitter.Address(), tx) //nolint: wrapcheckCommittable suggestion was skipped due to low confidence.
Tools
GitHub Check: Lint (services/rfq)
[failure] 181-181:
error returned from external package is unwrapped: sig: field Signer github.com/ethereum/go-ethereum/accounts/abi/bind.SignerFn (wrapcheck)
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced
Withdrawmethod in/services/rfq/relayer/relapi/handler.gofor token withdrawals. - Added error handling for
TestEthWithdrawandTestERC20Withdrawin/services/rfq/relayer/relapi/server_test.go. - Validated token and withdrawal addresses in
Withdrawmethod. - Enhanced transaction submission for gas tokens and ERC20 tokens.
- Improved error reporting in withdrawal tests.
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
|
|
||
| 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) |
There was a problem hiding this comment.
🪶 style: Consider adding error handling for the erc20Contract.Transfer call.
There was a problem hiding this comment.
Actionable comments posted: 12
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- services/rfq/relayer/relapi/handler.go (4 hunks)
- services/rfq/relayer/relapi/server_test.go (2 hunks)
Additional context used
GitHub Check: Lint (services/rfq)
services/rfq/relayer/relapi/server_test.go
[failure] 228-228:
error returned from interface method should be wrapped: sig: func (github.com/ethereum/go-ethereum.ChainStateReader).BalanceAt(ctx context.Context, account github.com/ethereum/go-ethereum/common.Address, blockNumber *math/big.Int) (*math/big.Int, error) (wrapcheck)
[failure] 264-264:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/testcontracts/mockerc20.MockERC20Caller).BalanceOf(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.CallOpts, account github.com/ethereum/go-ethereum/common.Address) (*math/big.Int, error) (wrapcheck)services/rfq/relayer/relapi/handler.go
[failure] 173-173:
if chain.IsGasToken(req.TokenAddress)has complex nested blocks (complexity: 5) (nestif)
[failure] 182-182:
error returned from external package is unwrapped: sig: field Signer github.com/ethereum/go-ethereum/accounts/abi/bind.SignerFn (wrapcheck)
GitHub Check: codecov/patch
services/rfq/relayer/relapi/handler.go
[warning] 148-150: services/rfq/relayer/relapi/handler.go#L148-L150
Added lines #L148 - L150 were not covered by tests
[warning] 154-156: services/rfq/relayer/relapi/handler.go#L154-L156
Added lines #L154 - L156 were not covered by tests
[warning] 160-162: services/rfq/relayer/relapi/handler.go#L160-L162
Added lines #L160 - L162 were not covered by tests
[warning] 169-171: services/rfq/relayer/relapi/handler.go#L169-L171
Added lines #L169 - L171 were not covered by tests
[warning] 185-187: services/rfq/relayer/relapi/handler.go#L185-L187
Added lines #L185 - L187 were not covered by tests
[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
[warning] 214-214: services/rfq/relayer/relapi/handler.go#L214
Added line #L214 was not covered by tests
[warning] 222-222: services/rfq/relayer/relapi/handler.go#L222
Added line #L222 was not covered by tests
[warning] 231-231: services/rfq/relayer/relapi/handler.go#L231
Added line #L231 was not covered by tests
[warning] 273-275: services/rfq/relayer/relapi/handler.go#L273-L275
Added lines #L273 - L275 were not covered by tests
Additional comments not posted (2)
services/rfq/relayer/relapi/handler.go (2)
31-36: LGTM!The
NewHandlerfunction has been updated correctly to include the new parameters.
122-129: LGTM!The variable renaming improves clarity.
| func (c *RelayerClientSuite) TestEthWithdraw() { | ||
| backend := c.underlying.testBackends[uint64(c.underlying.originChainID)] | ||
|
|
||
| startBalance, err := backend.BalanceAt(c.GetTestContext(), testWithdrawalAddress, nil) | ||
| c.Require().NoError(err) | ||
|
|
||
| withdrawalAmount := big.NewInt(50) | ||
|
|
||
| _, err = c.Client.Withdraw(c.GetTestContext(), &relapi.WithdrawRequest{ | ||
| ChainID: uint32(backend.GetChainID()), | ||
| To: testWithdrawalAddress, | ||
| Amount: withdrawalAmount.String(), | ||
| TokenAddress: chain.EthAddress, | ||
| }) | ||
| c.Require().NoError(err) | ||
|
|
||
| // Wait for the transaction to be mined | ||
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| balance, err := backend.BalanceAt(ctx, testWithdrawalAddress, nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | ||
|
|
||
| if balance.Cmp(expectedBalance) != 0 { | ||
| return fmt.Errorf("balance not updated") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| c.Require().NoError(err) | ||
| } |
There was a problem hiding this comment.
Wrap error returned from external package.
The error returned from the external package should be wrapped to provide more context.
- return err
+ return fmt.Errorf("failed to get balance: %w", err)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.
| func (c *RelayerClientSuite) TestEthWithdraw() { | |
| backend := c.underlying.testBackends[uint64(c.underlying.originChainID)] | |
| startBalance, err := backend.BalanceAt(c.GetTestContext(), testWithdrawalAddress, nil) | |
| c.Require().NoError(err) | |
| withdrawalAmount := big.NewInt(50) | |
| _, err = c.Client.Withdraw(c.GetTestContext(), &relapi.WithdrawRequest{ | |
| ChainID: uint32(backend.GetChainID()), | |
| To: testWithdrawalAddress, | |
| Amount: withdrawalAmount.String(), | |
| TokenAddress: chain.EthAddress, | |
| }) | |
| c.Require().NoError(err) | |
| // Wait for the transaction to be mined | |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| balance, err := backend.BalanceAt(ctx, testWithdrawalAddress, nil) | |
| if err != nil { | |
| return err | |
| } | |
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | |
| if balance.Cmp(expectedBalance) != 0 { | |
| return fmt.Errorf("balance not updated") | |
| } | |
| return nil | |
| }) | |
| c.Require().NoError(err) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("failed to get balance: %w", err) | |
| } |
Tools
GitHub Check: Lint (services/rfq)
[failure] 228-228:
error returned from interface method should be wrapped: sig: func (github.com/ethereum/go-ethereum.ChainStateReader).BalanceAt(ctx context.Context, account github.com/ethereum/go-ethereum/common.Address, blockNumber *math/big.Int) (*math/big.Int, error) (wrapcheck)
| func (c *RelayerClientSuite) TestERC20Withdraw() { | ||
| backend := c.underlying.testBackends[uint64(c.underlying.originChainID)] | ||
|
|
||
| _, erc20 := c.underlying.deployManager.GetMockERC20(c.GetTestContext(), backend) | ||
|
|
||
| startBalance, err := erc20.BalanceOf(&bind.CallOpts{Context: c.GetTestContext()}, testWithdrawalAddress) | ||
| c.Require().NoError(err) | ||
|
|
||
| withdrawalAmount := big.NewInt(50) | ||
|
|
||
| _, err = c.Client.Withdraw(c.GetTestContext(), &relapi.WithdrawRequest{ | ||
| ChainID: uint32(backend.GetChainID()), | ||
| To: testWithdrawalAddress, | ||
| Amount: withdrawalAmount.String(), | ||
| TokenAddress: erc20.Address(), | ||
| }) | ||
| c.Require().NoError(err) | ||
|
|
||
| // Wait for the transaction to be mined | ||
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | ||
|
|
||
| if balance.Cmp(expectedBalance) != 0 { | ||
| return fmt.Errorf("balance not updated") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| c.Require().NoError(err) | ||
| } |
There was a problem hiding this comment.
Wrap error returned from external package.
The error returned from the external package should be wrapped to provide more context.
- return err
+ return fmt.Errorf("failed to get balance: %w", err)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.
| func (c *RelayerClientSuite) TestERC20Withdraw() { | |
| backend := c.underlying.testBackends[uint64(c.underlying.originChainID)] | |
| _, erc20 := c.underlying.deployManager.GetMockERC20(c.GetTestContext(), backend) | |
| startBalance, err := erc20.BalanceOf(&bind.CallOpts{Context: c.GetTestContext()}, testWithdrawalAddress) | |
| c.Require().NoError(err) | |
| withdrawalAmount := big.NewInt(50) | |
| _, err = c.Client.Withdraw(c.GetTestContext(), &relapi.WithdrawRequest{ | |
| ChainID: uint32(backend.GetChainID()), | |
| To: testWithdrawalAddress, | |
| Amount: withdrawalAmount.String(), | |
| TokenAddress: erc20.Address(), | |
| }) | |
| c.Require().NoError(err) | |
| // Wait for the transaction to be mined | |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | |
| if err != nil { | |
| return err | |
| } | |
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | |
| if balance.Cmp(expectedBalance) != 0 { | |
| return fmt.Errorf("balance not updated") | |
| } | |
| return nil | |
| }) | |
| c.Require().NoError(err) | |
| } | |
| func (c *RelayerClientSuite) TestERC20Withdraw() { | |
| backend := c.underlying.testBackends[uint64(c.underlying.originChainID)] | |
| _, erc20 := c.underlying.deployManager.GetMockERC20(c.GetTestContext(), backend) | |
| startBalance, err := erc20.BalanceOf(&bind.CallOpts{Context: c.GetTestContext()}, testWithdrawalAddress) | |
| c.Require().NoError(err) | |
| withdrawalAmount := big.NewInt(50) | |
| _, err = c.Client.Withdraw(c.GetTestContext(), &relapi.WithdrawRequest{ | |
| ChainID: uint32(backend.GetChainID()), | |
| To: testWithdrawalAddress, | |
| Amount: withdrawalAmount.String(), | |
| TokenAddress: erc20.Address(), | |
| }) | |
| c.Require().NoError(err) | |
| // Wait for the transaction to be mined | |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | |
| if (err != nil) { | |
| return fmt.Errorf("failed to get balance: %w", err) | |
| } | |
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | |
| if balance.Cmp(expectedBalance) != 0 { | |
| return fmt.Errorf("balance not updated") | |
| } | |
| return nil | |
| }) | |
| c.Require().NoError(err) | |
| } |
Tools
GitHub Check: Lint (services/rfq)
[failure] 264-264:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/testcontracts/mockerc20.MockERC20Caller).BalanceOf(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.CallOpts, account github.com/ethereum/go-ethereum/common.Address) (*math/big.Int, error) (wrapcheck)
| 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) | ||
| } | ||
|
|
||
| return transactor.Signer(h.submitter.Address(), tx) | ||
| }) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not submit transaction: %s", err.Error())}) | ||
| return | ||
| } |
There was a problem hiding this comment.
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] 185-187: services/rfq/relayer/relapi/handler.go#L185-L187
Added lines #L185 - L187 were not covered by tests
GitHub Check: Lint (services/rfq)
[failure] 173-173:
if chain.IsGasToken(req.TokenAddress)has complex nested blocks (complexity: 5) (nestif)
[failure] 182-182:
error returned from external package is unwrapped: sig: field Signer github.com/ethereum/go-ethereum/accounts/abi/bind.SignerFn (wrapcheck)
| 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 | ||
| } | ||
|
|
||
| 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) | ||
| }) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not submit transaction: %s", err.Error())}) | ||
| return | ||
| } |
There was a problem hiding this comment.
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}) |
There was a problem hiding this comment.
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?
| // 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), | ||
| }) | ||
| } |
There was a problem hiding this comment.
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?
Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
withdrawCommandto CLI in/services/rfq/relayer/cmd/cmd.gofor handling token withdrawals. - Introduced
withdrawCLI command in/services/rfq/relayer/cmd/commands.gowith flags for relayer URL, chain ID, amount, token address, and recipient address. - Implemented
Withdrawfunction in/services/rfq/relayer/relapi/handler.gofor Ethereum and ERC20 token withdrawals, including validation and error handling. - Added tests for withdrawals in
/services/rfq/relayer/relapi/server_test.goto ensure successful operations and correct balance updates. - Enhanced test suite in
/services/rfq/relayer/relapi/suite_test.goto cover new withdrawal API functionality.
6 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings
services/rfq/relayer/cmd/commands.go
Outdated
|
|
||
| // runCommand runs the rfq relayer. | ||
| var withdrawCommand = &cli.Command{ | ||
| Name: "widthdraw", |
There was a problem hiding this comment.
📚 spelling: Typo in command name 'widthdraw'. Should be 'withdraw'.
services/rfq/relayer/cmd/commands.go
Outdated
| return fmt.Errorf("relayer URL is required") | ||
| } | ||
|
|
||
| withdrawer := relapi.NewRelayerClient(metrics.Get(), c.String(relayerURLFlag.Name)) |
There was a problem hiding this comment.
🧠 logic: The error check for 'withdrawer' creation is misplaced. It should be after the 'NewRelayerClient' call.
|
|
||
| _, err = withdrawer.Withdraw(c.Context, &withdrawRequest) | ||
| if err != nil { | ||
| return fmt.Errorf("could not start relayer: %w", err) |
There was a problem hiding this comment.
🪶 style: Error message should reflect the context of the operation, e.g., 'could not execute withdrawal'.
|
|
||
| // Withdraw withdraws tokens from the relayer. | ||
| // | ||
| //nolint:cyclop |
There was a problem hiding this comment.
🪶 style: Consider refactoring to reduce cyclomatic complexity.
| return | ||
| } | ||
|
|
||
| //nolint: nestif |
There was a problem hiding this comment.
🪶 style: Consider refactoring to reduce nesting.
| 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) |
There was a problem hiding this comment.
🪶 style: Consider adding error handling for the erc20Contract.Transfer call.
| c.Require().NoError(err) | ||
| } | ||
|
|
||
| func (c *RelayerClientSuite) TestEthWithdrawCLI() { |
There was a problem hiding this comment.
🪶 style: Consider adding a timeout for the retry logic to avoid potential infinite loops.
| c.Require().NotNil(res) | ||
| } | ||
|
|
||
| func (c *RelayerClientSuite) TestERC20WithdrawCLI() { |
There was a problem hiding this comment.
🪶 style: Consider adding a timeout for the retry logic to avoid potential infinite loops.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
withdrawCLI command in/services/rfq/relayer/cmd/commands.gofor Ethereum and ERC20 token withdrawals. - Introduced
TokenIDExistsfunction in/services/rfq/relayer/relapi/export_test.goto check token ID existence. - Added
TestTokenIDExistsin/services/rfq/relayer/relapi/server_test.gofor validating token IDs. - Standardized
TokenIDDelimiterand added withdrawal configurations in/services/rfq/relayer/relconfig/config.go.
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added valid hex address check in
DecodeTokenIDfunction (services/rfq/relayer/relconfig/config.go) - Introduced configuration fields for enabling API withdrawals and specifying a withdrawal whitelist (
services/rfq/relayer/relconfig/config.go) - Added
TestDecodeTokenIDto test various scenarios forDecodeTokenIDfunction (services/rfq/relayer/relconfig/config_test.go)
No major changes found since last review.
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 9
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
Files selected for processing (9)
- contrib/opbot/config/config.go (1 hunks)
- services/rfq/relayer/cmd/cmd.go (1 hunks)
- services/rfq/relayer/cmd/commands.go (2 hunks)
- services/rfq/relayer/relapi/export_test.go (1 hunks)
- services/rfq/relayer/relapi/handler.go (4 hunks)
- services/rfq/relayer/relapi/server_test.go (2 hunks)
- services/rfq/relayer/relapi/suite_test.go (7 hunks)
- services/rfq/relayer/relconfig/config.go (3 hunks)
- services/rfq/relayer/relconfig/config_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- contrib/opbot/config/config.go
Additional context used
Learnings (5)
services/rfq/relayer/relapi/export_test.go (1)
Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relconfig/config.go:163-181 Timestamp: 2024-07-02T14:44:18.051Z Learning: When adding the `DecodeTokenID` function in `services/rfq/relayer/relconfig/config.go`, ensure to cover it with unit tests to handle various scenarios.services/rfq/relayer/relapi/handler.go (6)
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.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:162-165 Timestamp: 2024-07-02T14:49:46.636Z Learning: When adding new validation logic in the `Withdraw` function, ensure to export helper functions like `toAddressIsWhitelisted` for direct unit testing.Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relconfig/config.go:163-181 Timestamp: 2024-07-02T14:44:18.051Z Learning: When adding the `DecodeTokenID` function in `services/rfq/relayer/relconfig/config.go`, ensure to cover it with unit tests to handle various scenarios.services/rfq/relayer/relapi/suite_test.go (2)
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.services/rfq/relayer/relapi/server_test.go (4)
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:49:46.636Z Learning: When adding new validation logic in the `Withdraw` function, ensure to export helper functions like `toAddressIsWhitelisted` for direct unit testing.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.services/rfq/relayer/relconfig/config_test.go (1)
Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relconfig/config.go:163-181 Timestamp: 2024-07-02T14:44:18.051Z Learning: When adding the `DecodeTokenID` function in `services/rfq/relayer/relconfig/config.go`, ensure to cover it with unit tests to handle various scenarios.
Additional comments not posted (10)
services/rfq/relayer/relapi/export_test.go (1)
8-11: EnsuretokenIDExistsis covered by tests.The
TokenIDExistsfunction relies on thetokenIDExistshelper function. Ensure that the helper function is covered by unit tests for various scenarios.services/rfq/relayer/cmd/commands.go (1)
75-124: Ensure thewithdrawCommandlogic is covered by tests.The
withdrawCommandfunction defines a new CLI command for handling withdrawals. Ensure that the command logic is covered by unit tests for various scenarios.Verification successful
The
withdrawCommandlogic is covered by tests.The
withdrawCommandfunctionality is tested within theRelayerClientSuitein theservices/rfq/relayer/relapi/server_test.gofile, ensuring various scenarios are covered.
services/rfq/relayer/relapi/server_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the withdrawCommand logic is covered by tests. # Test: Search for the usage of withdrawCommand in test files. Expect: Occurrences in test files. rg --type go --glob '*_test.go' $'withdrawCommand'Length of output: 49
Script:
#!/bin/bash # Description: Verify if the withdrawCommand logic is covered by tests. # Test: Search for the usage of "withdraw" in test files to find related test cases. rg --type go --glob '*_test.go' 'withdraw'Length of output: 1974
Script:
#!/bin/bash # Description: Verify if the withdrawCommand logic is covered by tests. # Test: Search for the usage of `Withdraw` function in test files to find related test cases. rg --type go --glob '*_test.go' 'Withdraw'Length of output: 5400
services/rfq/relayer/relapi/handler.go (4)
145-213: Ensure theWithdrawfunction is covered by tests.The
Withdrawfunction now includes new validation logic. Ensure that the function is covered by unit tests for various scenarios.
241-288: Ensure theWithdrawRequeststruct and its methods are covered by tests.The
WithdrawRequeststruct has new fields and JSON marshaling/unmarshaling methods. Ensure that the struct and its methods are covered by unit tests for various scenarios.
232-239: Ensure thetoAddressIsWhitelistedfunction is covered by tests.The
toAddressIsWhitelistedfunction checks if an address is whitelisted for withdrawals. Ensure that the function is covered by unit tests for various scenarios.
215-230: Ensure thetokenIDExistsfunction is covered by tests.The
tokenIDExistsfunction checks if a token ID exists in the config. Ensure that the function is covered by unit tests for various scenarios.Verification successful
Ensure the
tokenIDExistsfunction is covered by tests.The
tokenIDExistsfunction is indeed covered by theTestTokenIDExiststest inservices/rfq/relayer/relapi/server_test.go. This test includes various scenarios such as valid token addresses, invalid token addresses, different chain IDs, and edge cases.
services/rfq/relayer/relapi/server_test.go: TheTestTokenIDExistsfunction covers multiple scenarios, ensuring comprehensive testing oftokenIDExists.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the tokenIDExists function is covered by tests. # Test: Search for the usage of tokenIDExists in test files. Expect: Occurrences in test files. rg --type go --glob '*_test.go' $'tokenIDExists'Length of output: 140
Script:
#!/bin/bash # Description: Check the content of export_test.go to verify if tokenIDExists is covered by tests. # Display the content of the test file. cat services/rfq/relayer/relapi/export_test.goLength of output: 385
Script:
#!/bin/bash # Description: Verify if the TokenIDExists function is covered by tests. # Test: Search for the usage of TokenIDExists in test files. Expect: Occurrences in test files. rg --type go --glob '*_test.go' $'TokenIDExists'Length of output: 476
Script:
#!/bin/bash # Description: Check the content of server_test.go to verify if TokenIDExists is covered by tests. # Display the content of the test file. cat services/rfq/relayer/relapi/server_test.goLength of output: 12707
services/rfq/relayer/relconfig/config.go (1)
164-186: LGTM! But verify code coverage.The code changes are approved.
Ensure the
DecodeTokenIDfunction is covered by tests.Verification successful
The
DecodeTokenIDfunction is covered by tests.
config_test.go: Contains theTestDecodeTokenIDfunction which testsDecodeTokenID.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `DecodeTokenID` function is covered by tests. # Test: Search for the function usage. Expect: Occurrences in test files. rg --type go $'DecodeTokenID'Length of output: 571
services/rfq/relayer/relapi/server_test.go (2)
281-328: LGTM!The
TestTokenIDExistsfunction correctly tests various scenarios for token ID existence.
7-13: LGTM!The
TestNewQuoterAPIServerfunction correctly starts the API server and checks its health status.services/rfq/relayer/relconfig/config_test.go (1)
460-504: LGTM!The
TestDecodeTokenIDfunction correctly tests various scenarios for theDecodeTokenIDfunction.
| @@ -23,7 +23,7 @@ func Start(args []string, buildInfo config.BuildInfo) { | |||
| } | |||
|
|
|||
| // commands | |||
| app.Commands = cli.Commands{runCommand} | |||
| app.Commands = cli.Commands{runCommand, withdrawCommand} | |||
There was a problem hiding this comment.
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 ofwithdrawCommand.
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
| 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, |
There was a problem hiding this comment.
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
NewHandlerfunction.
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
| // DecodeTokenID decodes a token ID into a chain ID and address. | ||
| func DecodeTokenID(id string) (chainID int, addr common.Address, err error) { | ||
| // defensive coding, first check if the token ID is valid | ||
| _, err = SanitizeTokenID(id) |
There was a problem hiding this comment.
Remove redundant validation in DecodeTokenID.
The call to SanitizeTokenID is unnecessary as the function performs its own validation.
- // defensive coding, first check if the token ID is valid
- _, err = SanitizeTokenID(id)
- if err != nil {
- return chainID, addr, err
- }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.
| // DecodeTokenID decodes a token ID into a chain ID and address. | |
| func DecodeTokenID(id string) (chainID int, addr common.Address, err error) { | |
| // defensive coding, first check if the token ID is valid | |
| _, err = SanitizeTokenID(id) | |
| // DecodeTokenID decodes a token ID into a chain ID and address. | |
| func DecodeTokenID(id string) (chainID int, addr common.Address, err error) { |
| for _, backend := range c.testBackends { | ||
| go func() { | ||
| defer wg.Done() | ||
| backend.FundAccount(c.GetTestContext(), c.wallet.Address(), *big.NewInt(params.Ether)) | ||
| }() | ||
|
|
||
| go func() { | ||
| defer wg.Done() | ||
| mockMetadata, mockToken := c.deployManager.GetMockERC20(c.GetTestContext(), backend) | ||
| auth := backend.GetTxContext(c.GetTestContext(), mockMetadata.OwnerPtr()).TransactOpts | ||
|
|
||
| tx, err := mockToken.Mint(auth, c.wallet.Address(), big.NewInt(1000000000000000000)) | ||
| c.Require().NoError(err) | ||
| backend.WaitForConfirmation(c.GetTestContext(), tx) | ||
| }() | ||
| } | ||
|
|
||
| server, err := relapi.NewRelayerAPI(c.GetTestContext(), c.cfg, c.handler, c.omniRPCClient, c.database, ts) | ||
| c.Require().NoError(err) | ||
| c.RelayerAPIServer = server | ||
| wg.Wait() | ||
| } |
There was a problem hiding this comment.
Add error handling for wg.Wait().
The function lacks error handling for wg.Wait(), which could lead to unhandled errors.
- wg.Wait()
+ if err := wg.Wait(); err != nil {
+ c.T().Fatal(err)
+ }Committable suggestion was skipped due to low confidence.
| chainID := chainID | ||
| // Setup Anvil backend for the suite to have RPC support | ||
| // anvilOpts := anvil.NewAnvilOptionBuilder() | ||
| // anvilOpts.SetChainID(chainID) | ||
| // anvilOpts.SetBlockTime(1 * time.Second) | ||
| // backend := anvil.NewAnvilBackend(c.GetSuiteContext(), c.T(), anvilOpts) | ||
| backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID)) | ||
|
|
||
| // add the backend to the list of backends | ||
| c.testBackends[chainID] = backend | ||
| c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend) | ||
| g.Go(func() error { | ||
| backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID)) | ||
|
|
||
| backend.FundAccount(c.GetSuiteContext(), c.testWallet.Address(), *big.NewInt(params.Ether)) | ||
|
|
||
| // add the backend to the list of backends | ||
| c.testBackendMux.Lock() | ||
| defer c.testBackendMux.Unlock() | ||
| c.testBackends[chainID] = backend | ||
| c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend) | ||
|
|
||
| return nil | ||
| }) |
There was a problem hiding this comment.
Fix the scope issue with the chainID variable.
The variable chainID should be passed as an argument to the function literal to avoid scope issues.
- for _, chainID := range chainIDs {
+ for _, id := range chainIDs {
// Setup Anvil backend for the suite to have RPC support
- g.Go(func() error {
+ g.Go(func(chainID uint64) func() error {
+ return func() error {
backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID))
backend.FundAccount(c.GetSuiteContext(), c.testWallet.Address(), *big.NewInt(params.Ether))
// add the backend to the list of backends
c.testBackendMux.Lock()
defer c.testBackendMux.Unlock()
c.testBackends[chainID] = backend
c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend)
return nil
}
- })
+ }(id))
}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.
| chainID := chainID | |
| // Setup Anvil backend for the suite to have RPC support | |
| // anvilOpts := anvil.NewAnvilOptionBuilder() | |
| // anvilOpts.SetChainID(chainID) | |
| // anvilOpts.SetBlockTime(1 * time.Second) | |
| // backend := anvil.NewAnvilBackend(c.GetSuiteContext(), c.T(), anvilOpts) | |
| backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID)) | |
| // add the backend to the list of backends | |
| c.testBackends[chainID] = backend | |
| c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend) | |
| g.Go(func() error { | |
| backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID)) | |
| backend.FundAccount(c.GetSuiteContext(), c.testWallet.Address(), *big.NewInt(params.Ether)) | |
| // add the backend to the list of backends | |
| c.testBackendMux.Lock() | |
| defer c.testBackendMux.Unlock() | |
| c.testBackends[chainID] = backend | |
| c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend) | |
| return nil | |
| }) | |
| for _, id := range chainIDs { | |
| // Setup Anvil backend for the suite to have RPC support | |
| g.Go(func(chainID uint64) func() error { | |
| return func() error { | |
| backend := geth.NewEmbeddedBackendForChainID(c.GetSuiteContext(), c.T(), new(big.Int).SetUint64(chainID)) | |
| backend.FundAccount(c.GetSuiteContext(), c.testWallet.Address(), *big.NewInt(params.Ether)) | |
| // add the backend to the list of backends | |
| c.testBackendMux.Lock() | |
| defer c.testBackendMux.Unlock() | |
| c.testBackends[chainID] = backend | |
| c.omniRPCTestBackends = append(c.omniRPCTestBackends, backend) | |
| return nil | |
| } | |
| }(id)) | |
| } |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | ||
| if err != nil { | ||
| return fmt.Errorf("could not fetch balance %w", err) | ||
| } | ||
|
|
||
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | ||
|
|
||
| if balance.Cmp(expectedBalance) != 0 { | ||
| return fmt.Errorf("balance not updated") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| c.Require().NoError(err) | ||
| c.Require().NotNil(res) | ||
| } |
There was a problem hiding this comment.
Add a timeout for the retry logic in TestERC20WithdrawCLI.
To avoid potential infinite loops, consider adding a timeout for the retry logic.
err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error {
balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress)
if err != nil {
return fmt.Errorf("could not fetch balance %w", err)
}
expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount)
if balance.Cmp(expectedBalance) != 0 {
return fmt.Errorf("balance not updated")
}
return nil
}, retry.WithMaxTotalTime(60*time.Second))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.
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | |
| if err != nil { | |
| return fmt.Errorf("could not fetch balance %w", err) | |
| } | |
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | |
| if balance.Cmp(expectedBalance) != 0 { | |
| return fmt.Errorf("balance not updated") | |
| } | |
| return nil | |
| }) | |
| c.Require().NoError(err) | |
| c.Require().NotNil(res) | |
| } | |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | |
| if err != nil { | |
| return fmt.Errorf("could not fetch balance %w", err) | |
| } | |
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | |
| if balance.Cmp(expectedBalance) != 0 { | |
| return fmt.Errorf("balance not updated") | |
| } | |
| return nil | |
| }, retry.WithMaxTotalTime(60*time.Second)) |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| balance, err := backend.BalanceAt(ctx, testWithdrawalAddress, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("could not fetch balance %w", err) | ||
| } | ||
|
|
||
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | ||
|
|
||
| if balance.Cmp(expectedBalance) != 0 { | ||
| return fmt.Errorf("balance not updated") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| c.Require().NoError(err) |
There was a problem hiding this comment.
Add a timeout for the retry logic in TestEthWithdraw.
To avoid potential infinite loops, consider adding a timeout for the retry logic.
err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error {
balance, err := backend.BalanceAt(ctx, testWithdrawalAddress, nil)
if err != nil {
return fmt.Errorf("could not fetch balance %w", err)
}
expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount)
if balance.Cmp(expectedBalance) != 0 {
return fmt.Errorf("balance not updated")
}
return nil
}, retry.WithMaxTotalTime(60*time.Second))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.
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| balance, err := backend.BalanceAt(ctx, testWithdrawalAddress, nil) | |
| if err != nil { | |
| return fmt.Errorf("could not fetch balance %w", err) | |
| } | |
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | |
| if balance.Cmp(expectedBalance) != 0 { | |
| return fmt.Errorf("balance not updated") | |
| } | |
| return nil | |
| }) | |
| c.Require().NoError(err) | |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| balance, err := backend.BalanceAt(ctx, testWithdrawalAddress, nil) | |
| if err != nil { | |
| return fmt.Errorf("could not fetch balance %w", err) | |
| } | |
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | |
| if balance.Cmp(expectedBalance) != 0 { | |
| return fmt.Errorf("balance not updated") | |
| } | |
| return nil | |
| }, retry.WithMaxTotalTime(60*time.Second)) | |
| c.Require().NoError(err) |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| status, err := c.underlying.database.SubmitterDB(). | ||
| GetNonceStatus( | ||
| ctx, | ||
| c.underlying.wallet.Address(), | ||
| big.NewInt(int64(c.underlying.originChainID)), | ||
| res.Nonce, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get status %w", err) | ||
| } | ||
|
|
||
| if status != submitterdb.Stored { | ||
| return fmt.Errorf("transaction not mined") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| c.Require().NoError(err) | ||
| c.Require().NotNil(res) | ||
| } |
There was a problem hiding this comment.
Add a timeout for the retry logic in TestEthWithdrawCLI.
To avoid potential infinite loops, consider adding a timeout for the retry logic.
err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error {
status, err := c.underlying.database.SubmitterDB().
GetNonceStatus(
ctx,
c.underlying.wallet.Address(),
big.NewInt(int64(c.underlying.originChainID)),
res.Nonce,
)
if err != nil {
return fmt.Errorf("could not get status %w", err)
}
if status != submitterdb.Stored {
return fmt.Errorf("transaction not mined")
}
return nil
}, retry.WithMaxTotalTime(60*time.Second))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.
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| status, err := c.underlying.database.SubmitterDB(). | |
| GetNonceStatus( | |
| ctx, | |
| c.underlying.wallet.Address(), | |
| big.NewInt(int64(c.underlying.originChainID)), | |
| res.Nonce, | |
| ) | |
| if err != nil { | |
| return fmt.Errorf("could not get status %w", err) | |
| } | |
| if status != submitterdb.Stored { | |
| return fmt.Errorf("transaction not mined") | |
| } | |
| return nil | |
| }) | |
| c.Require().NoError(err) | |
| c.Require().NotNil(res) | |
| } | |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| status, err := c.underlying.database.SubmitterDB(). | |
| GetNonceStatus( | |
| ctx, | |
| c.underlying.wallet.Address(), | |
| big.NewInt(int64(c.underlying.originChainID)), | |
| res.Nonce, | |
| ) | |
| if err != nil { | |
| return fmt.Errorf("could not get status %w", err) | |
| } | |
| if status != submitterdb.Stored { | |
| return fmt.Errorf("transaction not mined") | |
| } | |
| return nil | |
| }, retry.WithMaxTotalTime(60*time.Second)) | |
| c.Require().NoError(err) | |
| c.Require().NotNil(res) | |
| } |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get balance %w", err) | ||
| } | ||
|
|
||
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | ||
|
|
||
| if balance.Cmp(expectedBalance) != 0 { | ||
| return fmt.Errorf("balance not updated") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| c.Require().NoError(err) |
There was a problem hiding this comment.
Add a timeout for the retry logic in TestERC20Withdraw.
To avoid potential infinite loops, consider adding a timeout for the retry logic.
err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error {
balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress)
if err != nil {
return fmt.Errorf("could not get balance %w", err)
}
expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount)
if balance.Cmp(expectedBalance) != 0 {
return fmt.Errorf("balance not updated")
}
return nil
}, retry.WithMaxTotalTime(60*time.Second))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.
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | |
| if err != nil { | |
| return fmt.Errorf("could not get balance %w", err) | |
| } | |
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | |
| if balance.Cmp(expectedBalance) != 0 { | |
| return fmt.Errorf("balance not updated") | |
| } | |
| return nil | |
| }) | |
| c.Require().NoError(err) | |
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | |
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | |
| if err != nil { | |
| return fmt.Errorf("could not get balance %w", err) | |
| } | |
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | |
| if balance.Cmp(expectedBalance) != 0 { | |
| return fmt.Errorf("balance not updated") | |
| } | |
| return nil | |
| }, retry.WithMaxTotalTime(60*time.Second)) | |
| c.Require().NoError(err) |
There was a problem hiding this comment.
Actionable comments posted: 17
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
Files selected for processing (10)
- contrib/opbot/config/config.go (1 hunks)
- ethergo/submitter/submitter.go (2 hunks)
- services/rfq/relayer/cmd/cmd.go (1 hunks)
- services/rfq/relayer/cmd/commands.go (2 hunks)
- services/rfq/relayer/relapi/export_test.go (1 hunks)
- services/rfq/relayer/relapi/handler.go (4 hunks)
- services/rfq/relayer/relapi/server_test.go (2 hunks)
- services/rfq/relayer/relapi/suite_test.go (7 hunks)
- services/rfq/relayer/relconfig/config.go (3 hunks)
- services/rfq/relayer/relconfig/config_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- contrib/opbot/config/config.go
Files skipped from review as they are similar to previous changes (4)
- ethergo/submitter/submitter.go
- services/rfq/relayer/relapi/server_test.go
- services/rfq/relayer/relapi/suite_test.go
- services/rfq/relayer/relconfig/config.go
Additional context used
Learnings (4)
services/rfq/relayer/relapi/export_test.go (1)
Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relconfig/config.go:163-181 Timestamp: 2024-07-02T14:44:18.051Z Learning: When adding the `DecodeTokenID` function in `services/rfq/relayer/relconfig/config.go`, ensure to cover it with unit tests to handle various scenarios.services/rfq/relayer/cmd/commands.go (3)
Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relapi/handler.go:162-165 Timestamp: 2024-07-02T14:49:46.636Z Learning: When adding new validation logic in the `Withdraw` function, ensure to export helper functions like `toAddressIsWhitelisted` for direct unit testing.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.services/rfq/relayer/relapi/handler.go (6)
Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relapi/handler.go:162-165 Timestamp: 2024-07-02T14:49:46.636Z Learning: When adding new validation logic in the `Withdraw` function, ensure to export helper functions like `toAddressIsWhitelisted` for direct unit testing.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: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: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.Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relconfig/config.go:163-181 Timestamp: 2024-07-02T14:44:18.051Z Learning: When adding the `DecodeTokenID` function in `services/rfq/relayer/relconfig/config.go`, ensure to cover it with unit tests to handle various scenarios.services/rfq/relayer/relconfig/config_test.go (1)
Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relconfig/config.go:163-181 Timestamp: 2024-07-02T14:44:18.051Z Learning: When adding the `DecodeTokenID` function in `services/rfq/relayer/relconfig/config.go`, ensure to cover it with unit tests to handle various scenarios.
GitHub Check: Lint (services/rfq)
services/rfq/relayer/relconfig/config_test.go
[failure] 494-494:
Using the variable on range scopettin function literal (scopelint)
[failure] 495-495:
Using the variable on range scopettin function literal (scopelint)
[failure] 499-499:
Using the variable on range scopettin function literal (scopelint)
Additional comments not posted (1)
services/rfq/relayer/relapi/handler.go (1)
254-266: Add tests forMarshalJSONmethod.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?
Skipped due to learnings
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.
| func TestDecodeTokenID(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| id string | ||
| wantChain int | ||
| wantAddr common.Address | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "valid token ID", | ||
| id: "1-0x1234567890abcdef1234567890abcdef12345678", | ||
| wantChain: 1, | ||
| wantAddr: common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678"), | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "invalid token ID format", | ||
| id: "1_0x1234567890abcdef1234567890abcdef12345678", | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "invalid chain ID", | ||
| id: "x-0x1234567890abcdef1234567890abcdef12345678", | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "invalid address", | ||
| id: "1-0x12345", | ||
| wantErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| gotChain, gotAddr, err := relconfig.DecodeTokenID(tt.id) | ||
| if tt.wantErr { | ||
| assert.Error(t, err) | ||
| } else { | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, tt.wantChain, gotChain) | ||
| assert.Equal(t, tt.wantAddr, gotAddr) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix variable scope issue in table-driven tests.
The variable tt is being used in the function literal, which can lead to unexpected behavior. Use a local variable to avoid this issue.
for _, tt := range tests {
tt := tt // create a new 'tt' variable for the function literal scope
t.Run(tt.name, func(t *testing.T) {
gotChain, gotAddr, err := relconfig.DecodeTokenID(tt.id)
if tt.wantErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
assert.Equal(t, tt.wantChain, gotChain)
assert.Equal(t, tt.wantAddr, gotAddr)
}
})
}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.
| func TestDecodeTokenID(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| id string | |
| wantChain int | |
| wantAddr common.Address | |
| wantErr bool | |
| }{ | |
| { | |
| name: "valid token ID", | |
| id: "1-0x1234567890abcdef1234567890abcdef12345678", | |
| wantChain: 1, | |
| wantAddr: common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678"), | |
| wantErr: false, | |
| }, | |
| { | |
| name: "invalid token ID format", | |
| id: "1_0x1234567890abcdef1234567890abcdef12345678", | |
| wantErr: true, | |
| }, | |
| { | |
| name: "invalid chain ID", | |
| id: "x-0x1234567890abcdef1234567890abcdef12345678", | |
| wantErr: true, | |
| }, | |
| { | |
| name: "invalid address", | |
| id: "1-0x12345", | |
| wantErr: true, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| gotChain, gotAddr, err := relconfig.DecodeTokenID(tt.id) | |
| if tt.wantErr { | |
| assert.Error(t, err) | |
| } else { | |
| assert.NoError(t, err) | |
| assert.Equal(t, tt.wantChain, gotChain) | |
| assert.Equal(t, tt.wantAddr, gotAddr) | |
| } | |
| }) | |
| } | |
| } | |
| for _, tt := range tests { | |
| tt := tt // create a new 'tt' variable for the function literal scope | |
| t.Run(tt.name, func(t *testing.T) { | |
| gotChain, gotAddr, err := relconfig.DecodeTokenID(tt.id) | |
| if tt.wantErr { | |
| assert.Error(t, err) | |
| } else { | |
| assert.NoError(t, err) | |
| assert.Equal(t, tt.wantChain, gotChain) | |
| assert.Equal(t, tt.wantAddr, gotAddr) | |
| } | |
| }) | |
| } |
Tools
GitHub Check: Lint (services/rfq)
[failure] 494-494:
Using the variable on range scopettin function literal (scopelint)
[failure] 495-495:
Using the variable on range scopettin function literal (scopelint)
[failure] 499-499:
Using the variable on range scopettin function literal (scopelint)
| // 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) | ||
| } |
There was a problem hiding this comment.
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?
| @@ -23,7 +23,7 @@ | |||
| } | |||
|
|
|||
| // commands | |||
| app.Commands = cli.Commands{runCommand} | |||
| app.Commands = cli.Commands{runCommand, withdrawCommand} | |||
There was a problem hiding this comment.
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?
| @@ -4,9 +4,11 @@ | |||
| import ( | |||
| "fmt" | |||
|
|
|||
| "github.com/ethereum/go-ethereum/common" | |||
There was a problem hiding this comment.
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?
| if c.String(relayerURLFlag.Name) == "" { | ||
| return fmt.Errorf("relayer URL is required") | ||
| } |
There was a problem hiding this comment.
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?
| // 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 | ||
| } | ||
|
|
||
| if prospectiveChainID == chainID && prospectiveAddress == tokenAddress { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
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 | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
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?
| cfg relconfig.Config | ||
| submitter submitter.TransactionSubmitter |
There was a problem hiding this comment.
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?
| // 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 | ||
| } | ||
|
|
||
| wr.TokenAddress = common.HexToAddress(aux.TokenAddress) | ||
| wr.To = common.HexToAddress(aux.To) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
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?
| 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 | ||
| } | ||
|
|
||
| // 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 | ||
| } | ||
|
|
||
| // 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 | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
|
|
||
| //nolint: nestif | ||
| 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 | ||
| }) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not submit transaction: %s", err.Error())}) | ||
| return | ||
| } | ||
| } 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 | ||
| } | ||
|
|
||
| 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) | ||
| }) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not submit transaction: %s", err.Error())}) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| c.JSON(http.StatusOK, gin.H{"nonce": nonce}) | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Removed indirect dependency
github.com/LK4d4/trylockfrom multiplego.modfiles. - Added
Blacklistfield tocontrib/screener-api/config/config.go. - Updated
contrib/opbot/botmd/commands.goto prevent link unfurling in Slack messages. - Introduced
Keys()method incore/mapmutex/mapmutex.goand added corresponding tests. - Enhanced
ethergo/submitterwith new metrics and methods for distinct chain IDs. - Added new Grafana dashboard configuration in
services/rfq/relayer/dashboards/dashboard.json.
43 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
ToAddressIsWhitelistedfunction inservices/rfq/relayer/relapi/export_test.gofor whitelisting checks. - Introduced new test cases for withdrawal API in
services/rfq/relayer/relapi/server_test.go. - Modified
TestDecodeTokenIDloop inservices/rfq/relayer/relconfig/config_test.gofor correct scoping.
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added tests for Ethereum and ERC20 token withdrawals in
services/rfq/relayer/relapi/server_test.go - Introduced tests for token ID decoding functionality
- Implemented tests for address whitelisting in withdrawal operations
- Ensured validation and balance updates in withdrawal tests
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Description
Add a withdrawal api to rfq
To do:
Future work:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation