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 106 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 181 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L181
Check warning on line 184 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L183-L184
Check warning on line 187 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L186-L187
Check warning on line 192 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L190-L192
Check warning on line 202 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L194-L202
Check warning on line 205 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L204-L205
Check warning on line 210 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L208-L210
Check warning on line 213 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L212-L213
Check warning on line 217 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L215-L217
Check warning on line 220 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L219-L220
Check warning on line 224 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L224
Check warning on line 227 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L227
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 SubscribeActiveQuotes implementation
The
SubscribeActiveQuotes
method is a good addition for real-time quote updates. However, there are several areas that could be improved:fmt.Println
calls with proper logging using thelogger
variable.Here's a suggested improvement:
These changes improve logging, add a timeout for the initial subscription, and ensure proper cleanup in case of errors.
📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: codecov/patch
Check warning on line 233 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L230-L233
Check warning on line 238 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L235-L238
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 248 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L240-L248
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 250 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L250
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 265 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L253-L265
Check warning on line 274 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L268-L274
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.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 300 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L277-L300
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 317 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L305-L317
Check warning on line 329 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L321-L329
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 334 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L331-L334
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 344 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L339-L344
Check warning on line 349 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L346-L349
Check warning on line 351 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L351
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 447 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L437-L447
Check warning on line 451 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L449-L451
Check warning on line 453 in services/rfq/api/client/client.go
Codecov / codecov/patch
services/rfq/api/client/client.go#L453
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