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
rfq: mimimal viable withdrawal api #2815
rfq: mimimal viable withdrawal api #2815
Changes from 2 commits
3b80d3f
9258369
a17db24
484a9a3
e83dd9d
a3c3516
84dcdd7
6d10be7
5ed6d9a
dce9713
2965bb7
5c4554a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 118 in services/rfq/relayer/relapi/client.go
Codecov / codecov/patch
services/rfq/relayer/relapi/client.go#L117-L118
Check warning on line 121 in services/rfq/relayer/relapi/client.go
Codecov / codecov/patch
services/rfq/relayer/relapi/client.go#L120-L121
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 the new fields in the
Handler
struct are properly initialized.The addition of new fields should be tested to ensure they are correctly initialized and used.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
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.
Tip
Codebase Verification
The
NewHandler
function is not covered by tests.The search through the test files did not yield any results for the
NewHandler
function, indicating that it is not covered by unit tests. Please ensure that unit tests are added to cover various scenarios for this function.NewHandler
function.Analysis chain
Ensure the
NewHandler
function is covered by tests.The
NewHandler
function now includes additional parameters. Ensure that the function is covered by unit tests for various scenarios.Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 44
Script:
Length of output: 75358
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 the
NewHandler
function is covered by tests.The addition of new parameters should be tested to ensure the function works as expected.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
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 the
chainHandler
variable is covered by tests.The renaming of the variable should be tested to ensure it does not introduce any issues.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Check warning on line 149 in services/rfq/relayer/relapi/handler.go
Codecov / codecov/patch
services/rfq/relayer/relapi/handler.go#L147-L149
Check warning on line 155 in services/rfq/relayer/relapi/handler.go
Codecov / codecov/patch
services/rfq/relayer/relapi/handler.go#L153-L155
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 tests for
tokenIDExists
validation.The validation of the token address in the
Withdraw
function is not covered by tests. Ensure that this validation is tested for various scenarios.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
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.
Sure
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.
@trajan0x, here are the unit tests for the
tokenIDExists
validation:This code tests various scenarios to ensure that the
tokenIDExists
function works as expected. Let me know if you need any further modifications or additional tests.Learnings added
Check warning on line 161 in services/rfq/relayer/relapi/handler.go
Codecov / codecov/patch
services/rfq/relayer/relapi/handler.go#L159-L161
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 tests for
toAddressIsWhitelisted
validation.The validation of the withdrawal address in the
Withdraw
function is not covered by tests. Ensure that this validation is tested for various scenarios.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
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.
can you generate?
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.
@trajan0x, sure! Here are the unit tests for the
toAddressIsWhitelisted
validation in theWithdraw
function:This test suite covers the validation of the withdrawal address in the
Withdraw
function by testing both whitelisted and non-whitelisted addresses.Learnings added
Learnings used
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.
running the whole api for this is crazy. export it
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.
@trajan0x, I understand your concern. We can export the
toAddressIsWhitelisted
function to facilitate direct testing. Here are the proposed changes:Export the
toAddressIsWhitelisted
function:Update the
Withdraw
function to use the exported function:Unit tests for the
ToAddressIsWhitelisted
function:This approach ensures that the
toAddressIsWhitelisted
function is tested directly without the need to run the entire API.Learnings added
Learnings used
Check warning on line 170 in services/rfq/relayer/relapi/handler.go
Codecov / codecov/patch
services/rfq/relayer/relapi/handler.go#L168-L170
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 tests for amount validation.
The validation of the amount in the
Withdraw
function is not covered by tests. Ensure that this validation is tested for various scenarios.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
Check failure on line 180 in services/rfq/relayer/relapi/handler.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.
Wrap error returned from external package.
The error returned from the external package should be wrapped to provide more context.
Tools
GitHub Check: Lint (services/rfq)
Check warning on line 191 in services/rfq/relayer/relapi/handler.go
Codecov / codecov/patch
services/rfq/relayer/relapi/handler.go#L183-L191
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 tests for gas token withdrawal.
The gas token withdrawal logic in the
Withdraw
function is not covered by tests. Ensure that this logic is tested for various scenarios.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
Check warning on line 200 in services/rfq/relayer/relapi/handler.go
Codecov / codecov/patch
services/rfq/relayer/relapi/handler.go#L193-L200
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 tests for successful withdrawal response.
The successful withdrawal response in the
Withdraw
function is not covered by tests. Ensure that this response is tested.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Check warning on line 212 in services/rfq/relayer/relapi/handler.go
Codecov / codecov/patch
services/rfq/relayer/relapi/handler.go#L212
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 tests for
tokenIDExists
function.The
tokenIDExists
function is not covered by tests. Ensure that this function is tested for various scenarios.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
Check warning on line 220 in services/rfq/relayer/relapi/handler.go
Codecov / codecov/patch
services/rfq/relayer/relapi/handler.go#L220
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 tests for
tokenIDExists
function.Ensure that this function is tested for various scenarios, such as valid token addresses, invalid token addresses, and edge cases.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
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 tests for
toAddressIsWhitelisted
function.The
toAddressIsWhitelisted
function is not covered by tests. Ensure that this function is tested for various scenarios.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Check warning on line 229 in services/rfq/relayer/relapi/handler.go
Codecov / codecov/patch
services/rfq/relayer/relapi/handler.go#L229
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 tests for
toAddressIsWhitelisted
function.Ensure that this function is tested for various scenarios, such as valid addresses, invalid addresses, and edge cases.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
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 tests for
MarshalJSON
method.The
MarshalJSON
method in theWithdrawRequest
struct is not covered by tests. Ensure that this method is tested for various scenarios.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Check warning on line 273 in services/rfq/relayer/relapi/handler.go
Codecov / codecov/patch
services/rfq/relayer/relapi/handler.go#L271-L273
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 tests for
UnmarshalJSON
method.Ensure that this method is tested for various scenarios, such as valid JSON inputs, invalid JSON inputs, and edge cases.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
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 tests for the withdrawal route.
The POST
/withdraw
route is only added ifEnableAPIWithdrawals
is true. Ensure that this route is covered by tests.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch