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 24 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.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 nil
wsURL
parameter inNewAuthenticatedClient
The function
NewAuthenticatedClient
accepts awsURL
pointer. Ensure that you handle cases wherewsURL
might benil
to prevent potential nil pointer dereferences when accessing it later in the code.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
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:
Committable suggestion
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.
Consider adding a
GetWebSocketPort
method with a default valueTo provide a default WebSocket port when none is specified in the configuration, consider adding a
GetWebSocketPort
method, similar toGetRelayAckTimeout
andGetMaxQuoteAge
.Add the following code:
This ensures your application has a default port for WebSocket connections, improving robustness.