Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(rfq): active quoting API #3128
feat(rfq): active quoting API #3128
Changes from 100 commits
bb18412
31e52d8
e8ab231
782cffd
6344a37
8aa16cb
4764926
afb2f19
e30cd63
3c10c02
bdae4ca
138297d
fc1ea97
6ae7a71
7cdcade
01d83dc
ee408a9
94a8f4d
c39d62c
6beb23a
fdf9d12
e23175f
4b99340
c557a28
888ce50
3293166
63f1a1e
594d6ea
7e7c5a1
7dcdf59
8cae8e4
94ee250
46d04e2
36701ba
2616b54
fe7a774
60db841
83b7f6d
32065ee
c5e9a00
e7d08e7
8e405e5
c6db31f
7812573
a01fb9a
c27ef32
b296da8
4384fb2
be695ea
5656bae
1c3870c
2051b30
f0928c4
4683974
33c24a3
7aee229
ff0aece
91c1bf5
cdee6ea
8ccbb3f
f2920e2
83a3603
f112235
292cd37
dd961c1
2368313
d7948d4
161ea2e
c240cd3
c8b5435
7fa8003
b05e6b7
f2a4be9
f203e7c
0835aae
2996aaa
89c565e
0a2b46a
8850cf0
2bae6b1
af384d4
3ae9552
d3f839f
925617e
7ff7c81
99c9d5c
6d6d172
c40dada
7878364
2c46bcb
1025c6c
65ddc92
16b3a5b
d71d686
a0591d6
3bc93ab
aa50d07
b4a25e1
26c6bbc
04ff76b
3324e53
cb7dde0
cbc6e18
5cb6050
e687ece
c8a5868
8bad457
7e88a97
526f2af
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider updating the logging package
The current implementation uses
github.com/ipfs/go-log
for logging. It's recommended to use a more modern and widely adopted logging library or the project's standard logging approach. This would help maintain consistency across the project and potentially avoid issues with outdated dependencies.Consider replacing
github.com/ipfs/go-log
with a more standard logging library such aslog
from the standard library, or popular third-party options likegithub.com/sirupsen/logrus
orgo.uber.org/zap
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider updating logging package
You're using
github.com/ipfs/go-log
for logging. It's recommended to use a more modern logging library or the project's standard logging approach to maintain consistency and potentially avoid issues with outdated dependencies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
PutQuote method calls may still use the old parameter type
*model.PutQuoteRequest
. Please update all instances to use*model.PutRelayerQuoteRequest
to ensure type consistency.services/rfq/relayer/quoter/quoter.go:817
services/rfq/api/client/client_test.go:20
services/rfq/api/client/client_test.go:111
services/rfq/api/client/client_test.go:148
🔗 Analysis chain
Verify the function signature change in the codebase.
The
PutQuote
method signature has been modified to accept a*model.PutRelayerQuoteRequest
instead of a*model.PutQuoteRequest
. Ensure that all calls to this method have been updated to pass the correct parameter type.Run the following script to verify the
PutQuote
method usage:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 456
Script:
Length of output: 536
Script:
Length of output: 821
Script:
Length of output: 569
Script:
Length of output: 641
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid redundancy in client fields.
The
clientImpl
struct defines its ownrClient
field while also embeddingUnauthenticatedClient
, which may already contain anrClient
field. This could lead to confusion or unintended behavior due to field shadowing.Consider removing the
rClient
field fromclientImpl
or accessing the embeddedrClient
through theUnauthenticatedClient
interface to maintain clarity.Check warning on line 82 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L82
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential error when setting auth header.
In the
OnBeforeRequest
callback, ifgetAuthHeader
returns an error, it propagates up and might cause the request to fail silently. Make sure to log the error or handle it appropriately to aid in debugging.Apply this diff to add error logging:
authHeader, err := getAuthHeader(request.Context(), reqSigner) if err != nil { + logger.Errorf("Failed to get auth header: %v", err) return fmt.Errorf("failed to get auth header: %w", err) }
📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: codecov/patch
Check warning on line 109 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L108-L109
Check warning on line 185 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L181-L185
Check warning on line 201 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L188-L201
Check warning on line 211 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L204-L211
Check warning on line 218 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L213-L218
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
logger.Errorf
instead oflogger.Error
for formatted error messagesAt line 217, you're using
logger.Error("Error running websocket listener: %s", err)
. Thelogger.Error
method may not support formatting verbs like%s
. Consider usinglogger.Errorf
to properly format the error message.🧰 Tools
🪛 GitHub Check: codecov/patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid capturing the outer
err
variable inside the goroutineIn the goroutine, you are assigning to the outer
err
variable, which is the named return parameter of theSubscribeActiveQuotes
function. This can lead to data races and confusion since the goroutine runs asynchronously and the error will not be propagated correctly to the caller.Apply this diff to declare a new local
err
variable inside the goroutine:📝 Committable suggestion
Check warning on line 221 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L221
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent potential goroutine leaks in
SubscribeActiveQuotes
.In the
SubscribeActiveQuotes
function, there are goroutines that may not properly exit if an error occurs or if the context is canceled, leading to resource leaks.Consider the following improvements:
defer
statements to ensure that all resources, such as the WebSocket connection and channels, are properly closed.Apply this diff to enhance resource cleanup:
🧰 Tools
🪛 GitHub Check: codecov/patch
Check warning on line 227 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L224-L227
Check warning on line 232 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L229-L232
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct the WebSocket URL scheme handling
The current method of replacing "http" with "ws" in the base URL can produce incorrect WebSocket URLs when using "https". For instance, "https://example.com" would become "ws://example.com", which is not secure. The correct secure WebSocket URL should be "wss://example.com".
Apply this diff to properly handle the URL scheme:
This change ensures that the WebSocket URL uses the correct scheme based on the original URL, supporting both
http
tows
andhttps
towss
conversions.📝 Committable suggestion
Check warning on line 242 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L234-L242
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent nil pointer dereference when closing
httpResp.Body
.In the
connectWebsocket
function,httpResp
may benil
upon a successful connection, as per thewebsocket.Dial
documentation. Attempting to closehttpResp.Body
without checking ifhttpResp
is notnil
could lead to a nil pointer dereference.Apply this diff to check for nil before closing:
📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: codecov/patch
Check warning on line 244 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L244
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve WebSocket connection handling
The
connectWebsocket
method establishes a WebSocket connection with proper headers. However, there are several areas that could be improved:httpResp.Body.Close()
is logged but not returned.Here's a suggested improvement:
These changes ensure proper handling of HTTPS URLs, add a timeout for the dial operation, and improve error handling when closing the response body.
📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: codecov/patch
Check warning on line 259 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L247-L259
Check warning on line 268 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L262-L268
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid variable shadowing when closing the connection
In the deferred function within
processWebsocket
, the variableerr
is redeclared, which shadows the named return parametererr
. This can lead to confusion or unintended behavior. Assign the result ofconn.Close()
to a new variable instead.Apply this diff to fix the variable shadowing:
🧰 Tools
GitHub Check: codecov/patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Respect context cancellation in
processWebsocket
.The
processWebsocket
function should respect the context cancellation and terminate gracefully when the context is done. Currently, it only checks for context cancellation in the main loop but not in the goroutines it spawns.Modify the
listenWsMessages
andsendPings
functions to accept the context and check for its cancellation:Then, update the
listenWsMessages
andsendPings
functions to check for context cancellation and return an error if the context is done.Check failure on line 273 in services/rfq/api/client/client.go
GitHub Actions / Lint (services/rfq)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle error when writing to websocket
When sending a message using
conn.WriteJSON(msg)
, if an error occurs, the loop continues without handling it appropriately. This could lead to silent failures or infinite loops on write errors.Consider returning the error to terminate the function or handle it as needed:
err := conn.WriteJSON(msg) if err != nil { return fmt.Errorf("error sending message to websocket: %w", err) + // Optionally, you might want to break or continue based on the error type. }
Check warning on line 294 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L271-L294
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve processWebsocket implementation
The
processWebsocket
method handles the main WebSocket communication loop effectively. However, there are several areas that could be improved:Here's a suggested improvement:
These changes improve error handling, ensure proper cleanup of goroutines, and provide more context in error messages.
📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: codecov/patch
Check warning on line 311 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L299-L311
Check warning on line 323 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L315-L323
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Respect context cancellation in
listenWsMessages
In the
listenWsMessages
function, ifconn.ReadMessage()
returns an error, the function exits without checking if the context has been canceled. Ensure that the function respects the context cancellation to prevent goroutine leaks.Modify the error handling to check for context cancellation:
📝 Committable suggestion
Check warning on line 328 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L325-L328
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve listenWsMessages implementation
The
listenWsMessages
method effectively listens for incoming WebSocket messages. However, there are several areas that could be improved:Here's a suggested improvement:
These changes add a read timeout, provide more detailed error logging, and ensure the method respects context cancellation. The
default
case in theselect
statement prevents blocking if thereadChan
is full.📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: codecov/patch
Check warning on line 338 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L333-L338
Check warning on line 343 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L340-L343
Check warning on line 345 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L345
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhance handleWsMessage implementation
The
handleWsMessage
method effectively unmarshals and handles incoming WebSocket messages. However, there are several areas that could be improved:Here's a suggested improvement:
These changes provide more detailed error messages, add message structure validation (assuming a
Validate
method exists onActiveRFQMessage
), and handle the case where the response channel might be full. Thedefault
case in theselect
statement prevents blocking ifrespChan
is full, logging a warning instead.Consider adding a
Validate
method to theActiveRFQMessage
struct if it doesn't already exist:🧰 Tools
🪛 GitHub Check: codecov/patch
Check warning on line 441 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L433-L441
Check warning on line 445 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L443-L445
Check warning on line 447 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L447
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve PutRFQRequest implementation
The
PutRFQRequest
method correctly implements the functionality for unauthenticated quote requests. However, there are several areas that could be improved:Here's a suggested improvement:
These changes provide more descriptive error messages and add a check for empty responses on successful requests, improving the robustness of the method.
Consider adding a validation step for the input
q *model.PutRFQRequest
to ensure all required fields are present before sending the request:This assumes a
Validate
method exists on thePutRFQRequest
struct. If it doesn't, consider adding one to perform input validation.🧰 Tools
🪛 GitHub Check: codecov/patch
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.