Rfq api/cli #2826
Rfq api/cli #2826
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update implements several enhancements across the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RelayerAPI
participant RelayerServer
participant Submitter
participant Blockchain
Client->>RelayerAPI: POST /withdraw (withdrawRequest)
RelayerAPI->>RelayerServer: Handle withdraw request
RelayerServer->>Submitter: Submit transaction (submit)
Submitter->>Blockchain: Send transaction
Blockchain->>Submitter: Transaction receipt
Submitter->>RelayerServer: Transaction success
RelayerServer->>RelayerAPI: Return withdraw response
RelayerAPI->>Client: WithdrawResponse (nonce)
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 (
|
Deploying sanguine-fe with
|
| Latest commit: |
97634de
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6be918b5.sanguine-fe.pages.dev |
| Branch Preview URL: | https://rfq-api-cli.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- 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/handler_test.go (1 hunks)
- services/rfq/relayer/relapi/server.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 (2 hunks)
- services/rfq/tools/withdrawal/cmd/cmd.go (1 hunks)
- services/rfq/tools/withdrawal/cmd/commands.go (1 hunks)
- services/rfq/tools/withdrawal/withdraw/withdraw.go (1 hunks)
- services/rfq/tools/withdrawal/withdraw/withdraw_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- services/rfq/tools/withdrawal/withdraw/withdraw_test.go
Additional context used
Learnings (2)
services/rfq/relayer/relapi/handler_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 (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.
Additional comments not posted (18)
services/rfq/tools/withdrawal/cmd/cmd.go (2)
12-34: LGTM!
19-19: Clarify the necessity of halting boot due to metrics.The TODO comment questions whether the boot process should be halted because of metrics setup. This needs to be addressed or clarified.
services/rfq/relayer/relapi/client.go (3)
20-20: LGTM!The
Withdrawmethod has been added to theRelayerClientinterface. Ensure that all implementations of this interface include this method.
105-108: LGTM!The
WithdrawResponsestruct has been introduced with aNoncefield.
110-123: LGTM!The
Withdrawmethod has been implemented in therelayerClientstruct. Ensure that all function calls toWithdrawmatch the new signature.services/rfq/relayer/relapi/server.go (3)
28-33: LGTM!The
RelayerAPIServerstruct now includes asubmitterfield.
103-103: LGTM!The new POST route
postWithdrawRoutehas been added.
Line range hint
111-124: LGTM!The new route
postWithdrawRoutehas been conditionally added based oncfg.EnableAPIWithdrawals.services/rfq/relayer/relapi/server_test.go (2)
208-240: LGTM!The
TestEthWithdrawfunction has been added to test Ethereum withdrawals.
242-276: LGTM!The
TestERC20Withdrawfunction has been added to test ERC20 token withdrawals.Verification successful
LGTM!
The
TestERC20WithdrawandTestEthWithdrawfunctions provide adequate test coverage for ERC20 token and ETH withdrawals, respectively.
TestERC20Withdrawtests ERC20 token withdrawals.TestEthWithdrawtests ETH withdrawals.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage for the `Withdraw` function. # Test: Search for the test functions. Expect: Coverage for all scenarios. rg --type go $'TestEthWithdraw|TestERC20Withdraw'Length of output: 236
Script:
#!/bin/bash # Extract the content of the TestEthWithdraw function from the server_test.go file. ast-grep --lang go --pattern $'func (c *$_) TestEthWithdraw() { $$$ }' services/rfq/relayer/relapi/server_test.goLength of output: 2641
services/rfq/relayer/relapi/handler.go (1)
144-206: LGTM!The
Withdrawfunction has been added to handle withdrawal requests. Ensure proper validation and error handling.services/rfq/relayer/relconfig/config.go (2)
54-57: LGTM!The new fields
EnableAPIWithdrawalsandWithdrawalWhitelistare well integrated into theConfigstruct.
163-181: LGTM!The
DecodeTokenIDfunction is well-implemented with proper validation and error handling.services/rfq/relayer/relapi/suite_test.go (3)
58-59: LGTM!The new variable
testWithdrawalAddressis well integrated and seems to be used for testing withdrawals.
100-118: LGTM!The new configurations for
EnableAPIWithdrawalsandWithdrawalWhitelistare well integrated into theSetupTestmethod.
Line range hint
167-261: LGTM!The new configurations and setups for mock chains and backends are well integrated into the
SetupSuitemethod.ethergo/submitter/submitter.go (2)
42-42: LGTM!The
meterNameconstant definition is straightforward and appropriate for metrics.
54-55: LGTM!The
Addressmethod addition to theTransactionSubmitterinterface and its implementation intxSubmitterImplare straightforward and appropriate.Also applies to: 688-690
| for _, req := range withdrawRequests { | ||
| res, err := w.client.Withdraw(ctx, &req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| responses = append(responses, res) | ||
| } | ||
|
|
||
| return responses, nil |
There was a problem hiding this comment.
Consider using goroutines for concurrent withdrawals.
To improve performance, consider using goroutines to handle multiple withdrawal requests concurrently.
var (
responses []*relapi.WithdrawResponse
wg sync.WaitGroup
mu sync.Mutex
errChan = make(chan error, len(withdrawRequests))
)
for _, req := range withdrawRequests {
wg.Add(1)
go func(req relapi.WithdrawRequest) {
defer wg.Done()
res, err := w.client.Withdraw(ctx, &req)
if err != nil {
errChan <- fmt.Errorf("withdrawal failed for request: %+v, error: %w", req, err)
return
}
mu.Lock()
responses = append(responses, res)
mu.Unlock()
}(req)
}
wg.Wait()
close(errChan)
if len(errChan) > 0 {
return nil, <-errChan
}
return responses, nil| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Enhance error handling with more context.
The error handling in the Withdraw method can be improved by providing more context about which request failed.
- if err != nil {
- return nil, err
+ if err != nil {
+ return nil, fmt.Errorf("withdrawal failed for request: %+v, error: %w", req, 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.
| if err != nil { | |
| return nil, err | |
| } | |
| if err != nil { | |
| return nil, fmt.Errorf("withdrawal failed for request: %+v, error: %w", req, err) | |
| } |
| func TestWithdrawRequestJSON(t *testing.T) { | ||
| original := relapi.WithdrawRequest{ | ||
| ChainID: 1, | ||
| Amount: "100", | ||
| TokenAddress: common.HexToAddress("0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE"), | ||
| To: common.HexToAddress("0xDeaDbeefdEAdbeefdEadbEEFdeadbeEFdEaDbeeF"), | ||
| } | ||
|
|
||
| // Marshal to JSON | ||
| data, err := json.Marshal(original) | ||
| assert.NoError(t, err) | ||
|
|
||
| // Unmarshal back to struct | ||
| var unmarshalled relapi.WithdrawRequest | ||
| err = json.Unmarshal(data, &unmarshalled) | ||
| assert.NoError(t, err) | ||
|
|
||
| // Check if the original and unmarshalled structs are the same | ||
| assert.Equal(t, original, unmarshalled) | ||
|
|
||
| // Check the JSON string explicitly | ||
| expectedJSON := `{"chain_id":1,"amount":"100","token_address":"0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee","to":"0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"}` | ||
| assert.JSONEq(t, expectedJSON, string(data)) | ||
| } |
There was a problem hiding this comment.
Add more test cases to cover edge cases and invalid inputs.
The current test covers basic JSON marshaling and unmarshaling. Consider adding more test cases to cover edge cases and invalid inputs.
func TestWithdrawRequestJSON(t *testing.T) {
testCases := []struct {
name string
original relapi.WithdrawRequest
expectedJSON string
expectError bool
}{
{
name: "valid request",
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"}`,
expectError: false,
},
{
name: "invalid amount",
original: relapi.WithdrawRequest{
ChainID: 1,
Amount: "invalid",
TokenAddress: common.HexToAddress("0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE"),
To: common.HexToAddress("0xDeaDbeefdEAdbeefdEadbEEFdeadbeEFdEaDbeeF"),
},
expectedJSON: `{"chain_id":1,"amount":"invalid","token_address":"0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee","to":"0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"}`,
expectError: true,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
data, err := json.Marshal(tc.original)
if tc.expectError {
assert.Error(t, err)
return
}
assert.NoError(t, err)
assert.JSONEq(t, tc.expectedJSON, string(data))
var unmarshalled relapi.WithdrawRequest
err = json.Unmarshal(data, &unmarshalled)
assert.NoError(t, err)
assert.Equal(t, tc.original, unmarshalled)
})
}
}| withdrawRequest := relapi.WithdrawRequest{ | ||
| ChainID: uint32(c.Uint(chainIDFlag.Name)), | ||
| Amount: c.String(amountFlag.Name), | ||
| TokenAddress: common.HexToAddress(c.String(tokenAddressFlag.Name)), | ||
| To: common.HexToAddress(toFlag.Name), | ||
| } |
There was a problem hiding this comment.
Validate CLI flag inputs.
Add validation for CLI flag inputs to ensure they are not empty and are in the correct format.
if c.String(relayerURLFlag.Name) == "" {
return fmt.Errorf("relayer URL is required")
}
chainID := c.Uint(chainIDFlag.Name)
if chainID == 0 {
return fmt.Errorf("valid chain ID is required")
}
amount := c.String(amountFlag.Name)
if amount == "" {
return fmt.Errorf("amount is required")
}
tokenAddress := c.String(tokenAddressFlag.Name)
if !common.IsHexAddress(tokenAddress) {
return fmt.Errorf("valid token address is required")
}
to := c.String(toFlag.Name)
if !common.IsHexAddress(to) {
return fmt.Errorf("valid recipient address is required")
}| if err != nil { | ||
| return fmt.Errorf("could not start relayer: %w", err) | ||
| } |
There was a problem hiding this comment.
Improve error handling.
The error message should indicate that the withdrawal request failed, not that the relayer could not start.
- return fmt.Errorf("could not start relayer: %w", err)
+ return fmt.Errorf("withdrawal request failed: %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.
| if err != nil { | |
| return fmt.Errorf("could not start relayer: %w", err) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("withdrawal request failed: %w", err) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
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 (2)
- contrib/opbot/config/config.go (1 hunks)
- services/rfq/tools/withdrawal/withdraw/withdraw.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/tools/withdrawal/withdraw/withdraw.go
Additional comments not posted (2)
contrib/opbot/config/config.go (2)
18-18: Ensure consistent formatting of YAML tags.The alignment of the YAML tag for
SignozBaseURLhas been corrected. This enhances readability and consistency.
20-20: Ensure consistent formatting of YAML tags.The alignment of the YAML tag for
RelayerURLShas been corrected. This enhances readability and consistency.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix/rfq-api #2826 +/- ##
=====================================================
+ Coverage 25.60195% 25.72228% +0.12033%
=====================================================
Files 765 708 -57
Lines 54863 52196 -2667
Branches 80 80
=====================================================
- Hits 14046 13426 -620
+ Misses 39368 37390 -1978
+ Partials 1449 1380 -69
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Added comments to
SignozBaseURLandRelayerURLSfields in/contrib/opbot/config/config.go - Introduced
runWithdrawCommandin/services/rfq/relayer/cmd/cmd.go - New CLI tool for withdrawals in
/services/rfq/relayer/cmd/commands.go - Added
Withdrawmethod in/services/rfq/relayer/relapi/handler.go - New test functions for withdrawals in
/services/rfq/relayer/relapi/server_test.go
7 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Updated
services/rfq/relayer/cmd/commands.goto align withdrawal functionality withrelapipackage. - Introduced
Withdrawerinterface inservices/rfq/relayer/relapi/suite_test.goand updated test setup for withdrawals. - Moved
withdraw.gotoservices/rfq/relayer/relapi, centralizing withdrawal functionality withinrelapipackage.
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)
- Introduced
Withdrawerinterface inservices/rfq/relayer/relapi/withdraw.go - Implemented
NewWithdrawerconstructor andWithdrawmethod - Enhanced
relapipackage with ERC20 and native token withdrawal functionalities
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| func (w *withdrawerImpl) Withdraw(ctx context.Context, withdrawRequest WithdrawRequest) (*WithdrawResponse, error) { | ||
| res, err := w.client.Withdraw(ctx, &withdrawRequest) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not withdraw: %w", err) |
There was a problem hiding this comment.
🪶 style: Error message could be more descriptive.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
runWithdrawCommandfor initiating withdrawals via CLI inservices/rfq/relayer/cmd/commands.go - Introduced necessary flags and validation checks for the withdrawal process
- Created
Withdrawerclient to handle withdrawal requests
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
runWithdrawCommandfor initiating withdrawals via CLI inservices/rfq/relayer/cmd/commands.go - Introduced flags for relayer URL, chain ID, amount, token address, and recipient address
- Implemented input validation for withdrawal requests
- Created
Withdrawerclient to handle withdrawal requests
1 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
|
|
||
| // runCommand runs the rfq relayer. | ||
| var runWithdrawCommand = &cli.Command{ | ||
| Name: "widthdraw", |
There was a problem hiding this comment.
📚 spelling: Typo in command name 'widthdraw'.
| Name: "widthdraw", | |
| Name: "withdraw", |
services/rfq/relayer/cmd/commands.go
Outdated
| withdrawer := relapi.NewWithdrawer(relapi.NewRelayerClient(metrics.Get(), c.String(relayerURLFlag.Name))) | ||
| if err != nil { | ||
| return fmt.Errorf("could not create relayer: %w", err) | ||
| } |
There was a problem hiding this comment.
🧠 logic: The error check here is redundant since withdrawer is always created successfully.
| if c.String(relayerURLFlag.Name) == "" { | ||
| return fmt.Errorf("relayer URL is required") | ||
| } |
There was a problem hiding this comment.
🪶 style: Consider moving this check before creating the withdrawer to avoid unnecessary object creation.
|
|
||
| // TODO: support multiple withdraw requests in one cli command (via config?) | ||
| // Withdraw withdraws the given amount of tokens to the given address. | ||
| func (w *withdrawerImpl) Withdraw(ctx context.Context, withdrawRequest WithdrawRequest) (*WithdrawResponse, error) { |
There was a problem hiding this comment.
What's the point of abstracting this out? Even if you were going to do a seperate interface still no reason for this
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced
withdrawCommandinservices/rfq/relayer/cmd/cmd.gofor CLI withdrawals - Added
Withdrawmethod,WithdrawRequest, andWithdrawResponsestructs inservices/rfq/relayer/cmd/commands.go - Removed
withdrawerfield fromRelayerClientSuiteinservices/rfq/relayer/relapi/suite_test.go - Deleted
services/rfq/relayer/relapi/withdraw.goand restructured withdrawal functionality
4 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
|
|
||
| // runCommand runs the rfq relayer. | ||
| var withdrawCommand = &cli.Command{ | ||
| Name: "widthdraw", |
There was a problem hiding this comment.
📚 spelling: Typo in command name 'widthdraw'.
| Name: "widthdraw", | |
| Name: "withdraw", |
| if err != nil { | ||
| return fmt.Errorf("could not create relayer: %w", err) | ||
| } |
There was a problem hiding this comment.
🧠 logic: The error check here is redundant since withdrawer is always created successfully.
| if c.String(relayerURLFlag.Name) == "" { | ||
| return fmt.Errorf("relayer URL is required") | ||
| } |
There was a problem hiding this comment.
🪶 style: Consider moving this check before creating the withdrawer to avoid unnecessary object creation.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Updated
Withdrawmethod calls inservices/rfq/relayer/relapi/server_test.goto useClientobject instead ofwithdrawerobject. - Ensure alignment with new withdrawal functionality in relayer client.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
Withdrawmethod and correspondingWithdrawRequestandWithdrawResponsestructs.Enhancements
RelayerAPIServerandHandlerstructures to support the new withdrawal functionalities.Configuration