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-guard): v2 guard logic [SLT-387] #3324
feat(rfq-guard): v2 guard logic [SLT-387] #3324
Changes from 7 commits
44b4bfc
abfca0e
e678664
bdfb0a4
135f30e
43b63ac
bcb5b77
f8fe3e6
61175b3
88fcf4f
873659e
c664fa8
d4bd5f1
e1b452c
1982bee
1570986
a677ffe
4fe0be0
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.
Ensure Consistent Types for RFQAddressV1 and RFQAddressV2
RFQAddressV1
is declared as a pointer to string (*string
), whereasRFQAddressV2
is declared as a string (string
). This inconsistency may lead to confusion and potential errors in handling these fields. If both addresses are required configurations, consider usingstring
for both. IfRFQAddressV1
is optional and may be nil, ensure this distinction is clearly documented and consistently handled throughout the codebase.Suggested Change:
Option 1: If both addresses are required:
Option 2: If both addresses can be optional:
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.
@dwasse I agree with the Rabbit that highlighting the fields as optional/required would be helpful here.
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.
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
Standardize Return Types of GetRFQAddress Methods
GetRFQAddressV1
returns a pointer to string(*string, error)
, whileGetRFQAddressV2
returns a string(string, error)
. This inconsistency might cause confusion and complicate error handling. Consider standardizing the return types of both methods to improve consistency and reduce potential errors.Suggested Change:
Option 1: Have both methods return
(*string, error)
:Option 2: Have both methods return
(string, error)
and adjust handling of optional values accordingly.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
Add null check for v2 address and consider refactoring initialization
Apply this pattern for v2:
Consider extracting the common initialization logic into a helper function:
Also applies to: 93-115
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-64: services/rfq/guard/service/guard.go#L60-L64
Added lines #L60 - L64 were not covered by tests
[warning] 67-90: services/rfq/guard/service/guard.go#L67-L90
Added lines #L67 - L90 were not covered by tests
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
Refactor initialization logic to eliminate duplication
The initialization code for V1 and V2 contracts is nearly identical, except for the contract versions and addresses. Extracting the shared logic into a helper function would reduce code duplication and improve maintainability.
Apply this refactor by creating a helper function:
Then, refactor the original code to use this helper function.
Also applies to: 94-115
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.
Add null check for V2 RFQ address
The V2 initialization does not check if
rfqAddrV2
is empty or nil before proceeding. This could lead to runtime errors if the V2 address is not set. Add a null check similar to the V1 initialization to ensure robustness.Apply this diff to add the null check:
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.
Add null check for v2 address
The v2 contract initialization assumes rfqAddrV2 is always non-empty. Consider adding a null check similar to v1 to handle cases where v2 contracts might not be deployed yet.
Apply this pattern:
📝 Committable suggestion
Check failure on line 319 in services/rfq/guard/service/guard.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.
🛠️ Refactor suggestion
Consider caching the v2 address lookup
The
isV2Address
function performs a config lookup and address comparison on every call. Consider caching the v2 addresses in a map during initialization to improve performance.Example implementation: