TestGetOptimalQuoteInGivenOut: Tests #399
Conversation
Cover `TestGetOptimalQuoteInGivenOut` with tests
WalkthroughThe changes entail modifications to various files within the routing functionality of a decentralized finance application. Key updates include enhancements to API documentation, introduction of new parameters for token transactions, restructuring of quote handling logic, and improvements in error management. Additionally, new test cases and utility functions have been added to bolster the testing framework. The modifications reflect a comprehensive update to the routing and quoting mechanisms, enhancing overall functionality and usability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RouterHandler
participant QuoteService
participant PoolManager
User->>RouterHandler: Request optimal quote
RouterHandler->>QuoteService: GetOptimalQuoteInGivenOut()
QuoteService->>PoolManager: Retrieve available pools
PoolManager-->>QuoteService: Return pool data
QuoteService-->>RouterHandler: Return quote details
RouterHandler-->>User: Respond with quote
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
Linter is complaining about following which stems from original |
deividaspetraitis
changed the title
There was a problem hiding this comment.
Thanks for separating this into a separate PR for ease of review.
What do you think about setting up our "mainnet state mock" test suite that creates a mock test environment from mainnet state rather than wiring a mock for GetOptimalQuote?
I know some time has been spent on the GetOptimalQuote mock but I'm not sure how I'm feeling about it due to the added wiring complexity to the core codebase while covering a few branches
Similar to this test. Note these lines that set up a mainnet environment.
In fact, it seems that your test already leverages a helper that sets up a router use case with the mainnet state. With the approach I suggest, we will not be able to cover this branch. However, that is acceptable because we will benefit from deeper functional testing while reducing complexity from not having to monkey-patch GetOptimalQuote
Let me know what you think
|
Thanks for taking your time to review and sharing thoughts! While this PR touches core logic and changes wiring I believe it will support long term project testability strategy, goals and general code base maintainability:
For example, for Please let me know if you still think we should go for |
Cover `TestGetOptimalQuoteInGivenOut` with tests: requested changes
|
@deividaspetraitis let's brainstorm this change sync sometime next week and align on direction. I think I propose kicking off a separate PR with the mainnet state test. Let me know what you think |
I have created separate PR with the mainnet state test #403 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (1)
router/usecase/routertesting/file.go (1)
5-10: Enhance error handling with additional context.While the function correctly handles errors, providing additional context in the error message can be helpful for debugging. Consider including the file path in the error message.
func (s *RouterTestHelper) MustReadFile(path string) string { b, err := os.ReadFile(path) - s.Require().NoError(err) + s.Require().NoError(err, "failed to read file: %s", path) return string(b) }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (39)
- .gitignore (1 hunks)
- docs/docs.go (1 hunks)
- docs/swagger.json (1 hunks)
- docs/swagger.yaml (1 hunks)
- domain/mocks/pool_mock.go (2 hunks)
- domain/mocks/router_usecase_mock.go (2 hunks)
- domain/mvc/router.go (1 hunks)
- domain/routable_pool.go (1 hunks)
- domain/router.go (2 hunks)
- domain/tokens.go (1 hunks)
- pools/usecase/export_test.go (1 hunks)
- router/delivery/http/http.go (1 hunks)
- router/delivery/http/router_handler.go (10 hunks)
- router/delivery/http/router_handler_test.go (4 hunks)
- router/types/errors.go (1 hunks)
- router/types/request.go (1 hunks)
- router/types/request_test.go (1 hunks)
- router/usecase/dynamic_splits.go (2 hunks)
- router/usecase/export_test.go (3 hunks)
- router/usecase/optimized_routes.go (1 hunks)
- router/usecase/optimized_routes_test.go (1 hunks)
- router/usecase/pools/routable_balancer_pool.go (3 hunks)
- router/usecase/pools/routable_concentrated_pool.go (3 hunks)
- router/usecase/pools/routable_cw_alloy_transmuter_pool.go (3 hunks)
- router/usecase/pools/routable_cw_orderbook_pool.go (3 hunks)
- router/usecase/pools/routable_cw_pool.go (2 hunks)
- router/usecase/pools/routable_cw_transmuter_pool.go (3 hunks)
- router/usecase/pools/routable_result_pool.go (5 hunks)
- router/usecase/pools/routable_stableswap_pool.go (2 hunks)
- router/usecase/quote_in_given_out.go (5 hunks)
- router/usecase/quote_out_given_in.go (1 hunks)
- router/usecase/quote_test.go (2 hunks)
- router/usecase/router_usecase.go (21 hunks)
- router/usecase/router_usecase_test.go (4 hunks)
- router/usecase/routertesting/file.go (1 hunks)
- router/usecase/routertesting/parsing/quote_amount_in_response.json (1 hunks)
- router/usecase/routertesting/parsing/quote_amount_out_response.json (1 hunks)
- router/usecase/routertesting/pool.go (1 hunks)
- router/usecase/routertesting/quote.go (1 hunks)
Files skipped from review due to trivial changes (3)
- .gitignore
- pools/usecase/export_test.go
- router/usecase/optimized_routes_test.go
Additional context used
GitHub Check: Run linter
router/usecase/routertesting/quote.go
[failure] 44-44:
(*RouterTestHelper).newRoutablePool-cosmWasmConfigalways receivesdomain.CosmWasmPoolRouterConfig{}(nil) (unparam)
Additional comments not posted (110)
router/delivery/http/http.go (2)
7-10: LGTM!The interface is correctly defined and follows Go's interface design principles.
The code changes are approved.
12-15: LGTM!The function is correctly implemented and leverages the interface method appropriately.
The code changes are approved.
router/types/errors.go (9)
7-7: LGTM!The error message is clear and descriptive.
The code changes are approved.
8-8: LGTM!The error message is clear and descriptive.
The code changes are approved.
9-9: LGTM!The error message is clear and descriptive.
The code changes are approved.
10-10: LGTM!The error message is clear and descriptive.
The code changes are approved.
11-11: LGTM!The error message is clear and descriptive.
The code changes are approved.
12-12: LGTM!The error message is clear and descriptive.
The code changes are approved.
13-13: LGTM!The error message is clear and descriptive.
The code changes are approved.
14-14: LGTM!The error message is clear and descriptive.
The code changes are approved.
15-15: LGTM!The error message is clear and descriptive.
The code changes are approved.
router/usecase/routertesting/parsing/quote_amount_out_response.json (1)
1-50: LGTM!The JSON structure is well-formed and consistent with the expected format for a quote amount out response.
The code changes are approved.
router/usecase/routertesting/parsing/quote_amount_in_response.json (1)
1-50: LGTM!The JSON structure is well-formed and consistent with the expected format for a quote amount in response.
The code changes are approved.
domain/routable_pool.go (3)
54-54: LGTM!The method
SetTokenOutDenom(denom string)enhances the flexibility of theRoutablePoolinterface by allowing the setting of the token out denomination.The code changes are approved.
56-56: LGTM!The method
GetTokenInDenom()enhances the functionality of theRoutablePoolinterface by allowing the retrieval of the token in denomination.The code changes are approved.
57-57: LGTM!The method
SetTokenInDenom(denom string)enhances the flexibility of theRoutablePoolinterface by allowing the setting of the token in denomination.The code changes are approved.
domain/tokens.go (1)
56-69: LGTM!The new enumeration type
TokenSwapMethodand its constants are well-defined and enhance the functionality related to token management.The code changes are approved.
router/usecase/quote_out_given_in.go (1)
18-28: LGTM!The
quoteExactAmountOutstruct is well-defined and includes necessary fields for exact out quotes.The code changes are approved.
router/types/request.go (2)
61-78: LGTM!The function is correctly implemented and handles the logic well.
The code changes are approved.
80-101: LGTM!The function is correctly implemented and handles the validation well.
The code changes are approved.
router/usecase/export_test.go (4)
18-18: LGTM!The alias change reflects a shift in the underlying implementation or intended use of the quote functionality.
The code changes are approved.
32-33: LGTM!The function implementation has been updated to enhance the routing logic by leveraging additional configuration and caching mechanisms.
The code changes are approved.
35-40: LGTM!The addition enhances the testability of the
routerUseCaseImplby providing a mechanism to control the behavior of quote retrieval during tests.The code changes are approved.
76-76: LGTM!The function implementation has been updated to improve the efficiency or accuracy of the ranking process by utilizing the
poolsUsecasedirectly.The code changes are approved.
router/usecase/pools/routable_balancer_pool.go (3)
23-24: LGTM!The addition of new fields enhances the struct's capability to manage multiple token denominations.
The code changes are approved.
44-46: LGTM!The function is correctly implemented and provides the necessary functionality.
The code changes are approved.
66-68: LGTM!The function is correctly implemented and provides the necessary functionality.
The code changes are approved.
router/usecase/pools/routable_stableswap_pool.go (2)
23-24: LGTM!The addition of
TokenInDenomand the update toTokenOutDenomwith theomitemptyJSON tag are appropriate and enhance the flexibility of the struct.The code changes are approved.
60-68: LGTM!The addition of
GetTokenInDenomandSetTokenInDenommethods is consistent with the existing method structure and enhances the functionality of the struct.The code changes are approved.
router/usecase/quote_in_given_out.go (11)
26-28: LGTM!The replacement of
quoteImplwithQuoteExactAmountOutandQuoteExactAmountInenhances clarity by explicitly distinguishing between the two quote types.The code changes are approved.
30-34: LGTM!The
NewQuoteExactAmountOutfunction correctly initializes and returns aquoteExactAmountOutinstance.The code changes are approved.
Line range hint
36-43: LGTM!The
quoteExactAmountInstruct is well-defined and includes fields necessary for handling quotes with exact amounts in.The code changes are approved.
Line range hint
54-110: LGTM!The
PrepareResultmethod is well-implemented, preparing the quote for output, stripping unnecessary fields, and computing the effective spread factor.The code changes are approved.
112-113: LGTM!The
GetAmountInmethod correctly returns theAmountInfield.The code changes are approved.
117-118: LGTM!The
GetAmountOutmethod correctly returns theAmountOutfield.The code changes are approved.
122-123: LGTM!The
GetRoutemethod correctly returns theRoutefield.The code changes are approved.
127-128: LGTM!The
GetEffectiveFeemethod correctly returns theEffectiveFeefield.The code changes are approved.
Line range hint
132-142: LGTM!The
Stringmethod provides a useful string representation of the quote for debugging and logging.The code changes are approved.
145-146: LGTM!The
GetPriceImpactmethod correctly returns thePriceImpactfield.The code changes are approved.
150-152: LGTM!The
GetInBaseOutQuoteSpotPricemethod correctly returns theInBaseOutQuoteSpotPricefield.The code changes are approved.
router/usecase/pools/routable_cw_transmuter_pool.go (2)
23-24: LGTM!The addition of
TokenInDenomand the update toTokenOutDenomwith theomitemptyJSON tag are appropriate and enhance the flexibility of the struct.The code changes are approved.
81-84: LGTM!The addition of
GetTokenInDenomandSetTokenInDenommethods is consistent with the existing method structure and enhances the functionality of the struct.The code changes are approved.
Also applies to: 117-120
router/usecase/routertesting/quote.go (4)
16-21: LGTM!The variables are correctly initialized with appropriate values.
The code changes are approved.
23-42: LGTM!The variables are correctly initialized with appropriate values.
The code changes are approved.
56-108: LGTM!The function is correctly implemented and uses helper functions and variables appropriately.
The code changes are approved.
110-158: LGTM!The function is correctly implemented and uses helper functions and variables appropriately.
The code changes are approved.
domain/mvc/router.go (1)
62-64: LGTM!The method is correctly added to the interface and enhances the functionality.
The code changes are approved.
router/usecase/pools/routable_result_pool.go (6)
32-32: LGTM!The update to include the
omitemptyJSON tag is appropriate and allows the field to be omitted from JSON output if it is empty.The code changes are approved.
33-33: LGTM!The addition of the field with the
omitemptyJSON tag is appropriate and allows the field to be serialized conditionally.The code changes are approved.
43-46: LGTM!The method is correctly implemented and enhances the mutability of the struct.
The code changes are approved.
48-51: LGTM!The method is correctly implemented and enhances the mutability of the struct.
The code changes are approved.
65-75: LGTM!The method is correctly implemented and expands the instantiation options for users of the struct.
The code changes are approved.
136-139: LGTM!The method is correctly implemented and ensures that the new field is accessible through the public interface of the struct.
The code changes are approved.
domain/mocks/router_usecase_mock.go (1)
68-73: LGTM!The function is correctly implemented with a panic for unimplemented cases.
The code changes are approved.
router/usecase/optimized_routes.go (1)
74-74: Verify the correctness of the new type.The
finalQuotevariable type has been changed fromquoteImpltoquoteExactAmountIn. Ensure that the new type is correct and compatible with the rest of the code.Run the following script to verify the usage of
quoteExactAmountIn:Verification successful
The new type
quoteExactAmountInis correct and well-integrated.The
quoteExactAmountIntype is extensively used across the codebase and is validated through unit tests. It implements the necessary methods and interfaces, indicating that the change fromquoteImpltoquoteExactAmountInis correct and compatible with the rest of the code.
- Verified in
router/usecase/router_usecase.go- Verified in
router/usecase/quote_in_given_out.go- Verified in
router/usecase/dynamic_splits.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `quoteExactAmountIn`. # Test: Search for the usage of `quoteExactAmountIn`. Expect: Correct usage and compatibility. rg --type go -A 5 $'quoteExactAmountIn'Length of output: 9753
router/usecase/pools/routable_cw_alloy_transmuter_pool.go (2)
87-90: LGTM!The function is correctly implemented and follows the pattern of other getter functions in the struct.
The code changes are approved.
109-112: LGTM!The function is correctly implemented and follows the pattern of other setter functions in the struct.
The code changes are approved.
domain/mocks/pool_mock.go (3)
27-27: LGTM!The addition of the
TokenInDenomfield is consistent with the existing structure.The code changes are approved.
130-133: LGTM!The
GetTokenInDenommethod correctly retrieves theTokenInDenomvalue.The code changes are approved.
135-138: LGTM!The
SetTokenInDenommethod correctly sets theTokenInDenomvalue.The code changes are approved.
router/usecase/dynamic_splits.go (2)
Line range hint
40-47: LGTM!The change in the
quotevariable type fromquoteImpltoquoteExactAmountInis consistent with the new logic.The code changes are approved.
Line range hint
173-179: LGTM!The change in the
quotevariable type fromquoteImpltoquoteExactAmountInis consistent with the new logic.The code changes are approved.
router/usecase/pools/routable_cw_pool.go (5)
33-33: LGTM!The addition of the
omitemptyJSON tag to theTokenOutDenomfield enhances the flexibility of the struct.The code changes are approved.
34-34: LGTM!The addition of the
TokenInDenomfield with theomitemptyJSON tag is consistent with the existing structure.The code changes are approved.
118-121: LGTM!The
SetTokenInDenommethod correctly sets theTokenInDenomvalue.The code changes are approved.
123-126: LGTM!The
SetTokenOutDenommethod correctly sets theTokenOutDenomvalue.The code changes are approved.
128-131: LGTM!The
GetTokenInDenommethod correctly retrieves theTokenInDenomvalue.The code changes are approved.
router/delivery/http/router_handler_test.go (1)
107-109: LGTM!The error handling modifications enhance the clarity and specificity of error reporting.
The code changes are approved.
Also applies to: 119-121, 131-133
domain/router.go (2)
25-27: LGTM!The new method
CalculateTokenOutByTokenInenhances the functionality of theRouteinterface.The code changes are approved.
Also applies to: 29-29
59-59: LGTM!The new method
GetInBaseOutQuoteSpotPriceenhances the functionality of theQuoteinterface.The code changes are approved.
router/types/request_test.go (3)
18-126: LGTM!The test function is well-structured and covers various scenarios.
The code changes are approved.
129-189: LGTM!The test function is well-structured and covers various scenarios.
The code changes are approved.
191-260: LGTM!The test function is well-structured and covers various scenarios.
The code changes are approved.
router/usecase/pools/routable_cw_orderbook_pool.go (4)
27-27: LGTM!The addition of the
TokenInDenomfield with theomitemptyJSON tag is appropriate for flexible serialization.The code changes are approved.
28-28: LGTM!The addition of the
omitemptyJSON tag to theTokenOutDenomfield is appropriate for flexible serialization.The code changes are approved.
152-154: LGTM!The function correctly retrieves the
TokenInDenomfield.The code changes are approved.
174-176: LGTM!The function correctly sets the
TokenInDenomfield.The code changes are approved.
router/usecase/pools/routable_concentrated_pool.go (4)
29-29: LGTM!The addition of the
TokenInDenomfield with theomitemptyJSON tag is appropriate for flexible serialization.The code changes are approved.
30-30: LGTM!The addition of the
omitemptyJSON tag to theTokenOutDenomfield is appropriate for flexible serialization.The code changes are approved.
202-204: LGTM!The function correctly retrieves the
TokenInDenomfield.The code changes are approved.
220-222: LGTM!The function correctly sets the
TokenInDenomfield.The code changes are approved.
router/usecase/quote_test.go (1)
Line range hint
5-200: LGTM!The refactoring of the
TestPrepareResultfunction to use helper methods for pool preparation and the table-driven format improves readability and maintainability.The code changes are approved.
docs/swagger.yaml (3)
167-167: LGTM!Making
tokenInoptional adds flexibility to the API.The code changes are approved.
168-171: LGTM!Adding
tokenInDenomenhances the API's ability to handle input tokens more precisely.The code changes are approved.
172-174: LGTM!Adding
tokenOutenhances the API's ability to handle output tokens more precisely.The code changes are approved.
router/delivery/http/router_handler.go (8)
19-19: LGTM!Importing the
typespackage is necessary for the new error handling mechanism.The code changes are approved.
Line range hint
61-131: LGTM!The refactoring of
GetOptimalQuoteto use a structured request object and new error handling improves maintainability and clarity.The code changes are approved.
84-86: LGTM!The removal of
getValidRoutingParametersis consistent with the shift towards a more encapsulated and request-driven approach.The code changes are approved.
316-316: LGTM!Using the new error types from the
typespackage aligns with the new error handling mechanism.The code changes are approved.
338-338: LGTM!Using the new error types from the
typespackage aligns with the new error handling mechanism.The code changes are approved.
353-353: LGTM!Using the new error types from the
typespackage aligns with the new error handling mechanism.The code changes are approved.
401-401: LGTM!Using the new error types from the
typespackage aligns with the new error handling mechanism.The code changes are approved.
401-401: LGTM!Using the new error types from the
typespackage aligns with the new error handling mechanism.The code changes are approved.
docs/swagger.json (4)
150-150: LGTM!Making
tokenInoptional adds flexibility to the API.The code changes are approved.
156-157: LGTM!Making
tokenOutDenomoptional adds flexibility to the API.The code changes are approved.
158-163: LGTM!Adding
tokenInDenomenhances the API's ability to handle input tokens more precisely.The code changes are approved.
164-168: LGTM!Adding
tokenOutenhances the API's ability to handle output tokens more precisely.The code changes are approved.
docs/docs.go (1)
159-177: LGTM!The changes enhance the flexibility of the API by allowing queries to be executed without specifying the
tokenInandtokenOutDenomparameters. The addition of new parameters provides additional context for the tokens involved in the transaction.The code changes are approved.
router/usecase/router_usecase.go (10)
110-128: LGTM!The introduction of the
GetOptimalQuoteFunctype alias and thegetOptimalQuoteFuncvariable enhances the flexibility and testability of the code.The code changes are approved.
142-155: LGTM!The refactoring of the
GetOptimalQuotemethod enhances the clarity of the code and separates the concerns of quote retrieval from the router use case implementation.The code changes are approved.
Line range hint
157-272: LGTM!The introduction of the
getOptimalQuotefunction centralizes the logic for obtaining optimal quotes, improving maintainability and readability.The code changes are approved.
281-297: LGTM!The new
GetOptimalQuoteInGivenOutmethod wraps the existingGetOptimalQuotefunctionality, ensuring that the underlying logic remains consistent while offering a new interface for users.The code changes are approved.
412-428: LGTM!The changes to the
rankRoutesByDirectQuotefunction streamline its operation and reduce dependencies on therouterUseCaseImplinstance.The code changes are approved.
Line range hint
443-506: LGTM!The changes to the
computeAndRankRoutesByDirectQuotefunction streamline its operation and reduce dependencies on therouterUseCaseImplinstance.The code changes are approved.
749-795: LGTM!The changes to the
handleCandidateRoutesfunction streamline its operation and reduce dependencies on therouterUseCaseImplinstance.The code changes are approved.
Line range hint
668-686: LGTM!The changes to the
getCachedCandidateRoutesfunction streamline its operation and reduce dependencies on therouterUseCaseImplinstance.The code changes are approved.
Line range hint
712-724: LGTM!The changes to the
getCachedRankedRoutesfunction streamline its operation and reduce dependencies on therouterUseCaseImplinstance.The code changes are approved.
930-936: LGTM!The new
convertMinTokensPoolLiquidityCapToFilterfunction provides a utility for converting the minimum tokens pool liquidity cap to a filter value, improving the flexibility of the code.The code changes are approved.
router/usecase/router_usecase_test.go (2)
627-644: LGTM!The function is correctly implemented and facilitates flexible testing.
The code changes are approved.
646-717: LGTM!The test method is well-structured and covers various scenarios effectively.
The code changes are approved.
| func (s *RouterTestHelper) PoolThree() (uint64, poolmanagertypes.PoolI) { | ||
| poolIDThree := s.PrepareCustomBalancerPool([]balancer.PoolAsset{ | ||
| { | ||
| Token: sdk.NewCoin(ETH, DefaultAmount), | ||
| Weight: sdk.NewInt(100), | ||
| }, | ||
| { | ||
| Token: sdk.NewCoin(USDC, DefaultAmount.MulRaw(4)), | ||
| Weight: sdk.NewInt(100), | ||
| }, | ||
| }, balancer.PoolParams{ | ||
| SwapFee: sdk.NewDecWithPrec(5, 3), | ||
| ExitFee: osmomath.ZeroDec(), | ||
| }) | ||
|
|
||
| poolThree, err := s.App.PoolManagerKeeper.GetPool(s.Ctx, poolIDThree) | ||
| s.Require().NoError(err) | ||
|
|
||
| return poolIDThree, poolThree |
There was a problem hiding this comment.
Add error handling for pool creation.
The function lacks error handling for the pool creation process. Consider adding error handling to ensure robustness.
poolIDThree := s.PrepareCustomBalancerPool([]balancer.PoolAsset{
// existing code
})
+if poolIDThree == 0 {
+ return 0, nil, errors.New("failed to create pool")
+}Committable suggestion was skipped due to low confidence.
| func (s *RouterTestHelper) PoolOne() (uint64, poolmanagertypes.PoolI) { | ||
| poolIDOne := s.PrepareCustomBalancerPool([]balancer.PoolAsset{ | ||
| { | ||
| Token: sdk.NewCoin(USDT, DefaultAmount.MulRaw(5)), | ||
| Weight: sdk.NewInt(100), | ||
| }, | ||
| { | ||
| Token: sdk.NewCoin(ETH, DefaultAmount), | ||
| Weight: sdk.NewInt(100), | ||
| }, | ||
| }, balancer.PoolParams{ | ||
| SwapFee: sdk.NewDecWithPrec(1, 2), | ||
| ExitFee: osmomath.ZeroDec(), | ||
| }) | ||
|
|
||
| poolOne, err := s.App.PoolManagerKeeper.GetPool(s.Ctx, poolIDOne) | ||
| s.Require().NoError(err) | ||
|
|
||
| return poolIDOne, poolOne |
There was a problem hiding this comment.
Add error handling for pool creation.
The function lacks error handling for the pool creation process. Consider adding error handling to ensure robustness.
poolIDOne := s.PrepareCustomBalancerPool([]balancer.PoolAsset{
// existing code
})
+if poolIDOne == 0 {
+ return 0, nil, errors.New("failed to create pool")
+}Committable suggestion was skipped due to low confidence.
| func (s *RouterTestHelper) PoolTwo() (uint64, poolmanagertypes.PoolI) { | ||
| poolIDTwo := s.PrepareCustomBalancerPool([]balancer.PoolAsset{ | ||
| { | ||
| Token: sdk.NewCoin(USDC, DefaultAmount), | ||
| Weight: sdk.NewInt(100), | ||
| }, | ||
| { | ||
| Token: sdk.NewCoin(USDT, DefaultAmount), | ||
| Weight: sdk.NewInt(100), | ||
| }, | ||
| }, balancer.PoolParams{ | ||
| SwapFee: sdk.NewDecWithPrec(3, 2), | ||
| ExitFee: osmomath.ZeroDec(), | ||
| }) | ||
|
|
||
| poolTwo, err := s.App.PoolManagerKeeper.GetPool(s.Ctx, poolIDTwo) | ||
| s.Require().NoError(err) | ||
|
|
||
| return poolIDTwo, poolTwo |
There was a problem hiding this comment.
Add error handling for pool creation.
The function lacks error handling for the pool creation process. Consider adding error handling to ensure robustness.
poolIDTwo := s.PrepareCustomBalancerPool([]balancer.PoolAsset{
// existing code
})
+if poolIDTwo == 0 {
+ return 0, nil, errors.New("failed to create pool")
+}Committable suggestion was skipped due to low confidence.
| // PrepareResult implements domain.Quote. | ||
| // PrepareResult mutates the quote to prepare | ||
| // it with the data formatted for output to the client. | ||
| // Specifically: | ||
| // It strips away unnecessary fields from each pool in the route. | ||
| // Computes an effective spread factor from all routes. | ||
| // | ||
| // Returns the updated route and the effective spread factor. | ||
| func (q *quoteExactAmountOut) PrepareResult(ctx context.Context, scalingFactor osmomath.Dec) ([]domain.SplitRoute, osmomath.Dec, error) { | ||
| // Prepare exact out in the quote for inputs inversion | ||
| if _, _, err := q.quoteExactAmountIn.PrepareResult(ctx, scalingFactor); err != nil { | ||
| return nil, osmomath.Dec{}, err | ||
| } | ||
|
|
||
| // Assign the inverted values to the quote | ||
| q.AmountOut = q.quoteExactAmountIn.AmountIn | ||
| q.AmountIn = q.quoteExactAmountIn.AmountOut | ||
| q.Route = q.quoteExactAmountIn.Route | ||
| q.EffectiveFee = q.quoteExactAmountIn.EffectiveFee | ||
| q.PriceImpact = q.quoteExactAmountIn.PriceImpact | ||
| q.InBaseOutQuoteSpotPrice = q.quoteExactAmountIn.InBaseOutQuoteSpotPrice | ||
|
|
||
| for i, route := range q.Route { | ||
| route, ok := route.(*RouteWithOutAmount) | ||
| if !ok { | ||
| return nil, osmomath.Dec{}, types.ErrInvalidRouteType | ||
| } | ||
|
|
||
| // invert the in and out amounts | ||
| route.InAmount, route.OutAmount = route.OutAmount, route.InAmount | ||
|
|
||
| q.Route[i] = route | ||
|
|
||
| // invert the in and out amounts for each pool | ||
| for _, p := range route.GetPools() { | ||
| p.SetTokenInDenom(p.GetTokenOutDenom()) | ||
| p.SetTokenOutDenom("") | ||
| } | ||
| } | ||
|
|
||
| return q.Route, q.EffectiveFee, nil |
There was a problem hiding this comment.
Add error handling for route type conversion.
The method lacks error handling for the route type conversion. Consider adding error handling to ensure robustness.
for i, route := range q.Route {
route, ok := route.(*RouteWithOutAmount)
if !ok {
- return nil, osmomath.Dec{}, types.ErrInvalidRouteType
+ return nil, osmomath.Dec{}, fmt.Errorf("invalid route type: %T", route)
}
// existing code
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // PrepareResult implements domain.Quote. | |
| // PrepareResult mutates the quote to prepare | |
| // it with the data formatted for output to the client. | |
| // Specifically: | |
| // It strips away unnecessary fields from each pool in the route. | |
| // Computes an effective spread factor from all routes. | |
| // | |
| // Returns the updated route and the effective spread factor. | |
| func (q *quoteExactAmountOut) PrepareResult(ctx context.Context, scalingFactor osmomath.Dec) ([]domain.SplitRoute, osmomath.Dec, error) { | |
| // Prepare exact out in the quote for inputs inversion | |
| if _, _, err := q.quoteExactAmountIn.PrepareResult(ctx, scalingFactor); err != nil { | |
| return nil, osmomath.Dec{}, err | |
| } | |
| // Assign the inverted values to the quote | |
| q.AmountOut = q.quoteExactAmountIn.AmountIn | |
| q.AmountIn = q.quoteExactAmountIn.AmountOut | |
| q.Route = q.quoteExactAmountIn.Route | |
| q.EffectiveFee = q.quoteExactAmountIn.EffectiveFee | |
| q.PriceImpact = q.quoteExactAmountIn.PriceImpact | |
| q.InBaseOutQuoteSpotPrice = q.quoteExactAmountIn.InBaseOutQuoteSpotPrice | |
| for i, route := range q.Route { | |
| route, ok := route.(*RouteWithOutAmount) | |
| if !ok { | |
| return nil, osmomath.Dec{}, types.ErrInvalidRouteType | |
| } | |
| // invert the in and out amounts | |
| route.InAmount, route.OutAmount = route.OutAmount, route.InAmount | |
| q.Route[i] = route | |
| // invert the in and out amounts for each pool | |
| for _, p := range route.GetPools() { | |
| p.SetTokenInDenom(p.GetTokenOutDenom()) | |
| p.SetTokenOutDenom("") | |
| } | |
| } | |
| return q.Route, q.EffectiveFee, nil | |
| for i, route := range q.Route { | |
| route, ok := route.(*RouteWithOutAmount) | |
| if !ok { | |
| return nil, osmomath.Dec{}, fmt.Errorf("invalid route type: %T", route) | |
| } | |
| // invert the in and out amounts | |
| route.InAmount, route.OutAmount = route.OutAmount, route.InAmount | |
| q.Route[i] = route | |
| // invert the in and out amounts for each pool | |
| for _, p := range route.GetPools() { | |
| p.SetTokenInDenom(p.GetTokenOutDenom()) | |
| p.SetTokenOutDenom("") | |
| } | |
| } | |
| return q.Route, q.EffectiveFee, nil |
| func (r *GetQuoteRequest) UnmarshalHTTPRequest(c echo.Context) error { | ||
| var err error | ||
| r.SingleRoute, err = domain.ParseBooleanQueryParam(c, "singleRoute") | ||
| if err != nil { | ||
| return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()}) | ||
| } | ||
|
|
||
| r.ApplyExponents, err = domain.ParseBooleanQueryParam(c, "applyExponents") | ||
| if err != nil { | ||
| return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()}) | ||
| } | ||
|
|
||
| if tokenIn := c.QueryParam("tokenIn"); tokenIn != "" { | ||
| tokenInCoin, err := sdk.ParseCoinNormalized(tokenIn) | ||
| if err != nil { | ||
| return ErrTokenInNotValid | ||
| } | ||
| r.TokenIn = &tokenInCoin | ||
| } | ||
|
|
||
| if tokenOut := c.QueryParam("tokenOut"); tokenOut != "" { | ||
| tokenOutCoin, err := sdk.ParseCoinNormalized(tokenOut) | ||
| if err != nil { | ||
| return ErrTokenOutNotValid | ||
| } | ||
| r.TokenOut = &tokenOutCoin | ||
| } | ||
|
|
||
| r.TokenInDenom = c.QueryParam("tokenInDenom") | ||
| r.TokenOutDenom = c.QueryParam("tokenOutDenom") | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Ensure consistent error handling.
The function currently has inconsistent error handling: some errors return a JSON response, others return an error directly. Consider standardizing the error handling approach.
Apply this diff to standardize the error handling:
func (r *GetQuoteRequest) UnmarshalHTTPRequest(c echo.Context) error {
var err error
r.SingleRoute, err = domain.ParseBooleanQueryParam(c, "singleRoute")
if err != nil {
- return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()})
+ return err
}
r.ApplyExponents, err = domain.ParseBooleanQueryParam(c, "applyExponents")
if err != nil {
- return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()})
+ return err
}
if tokenIn := c.QueryParam("tokenIn"); tokenIn != "" {
tokenInCoin, err := sdk.ParseCoinNormalized(tokenIn)
if err != nil {
- return ErrTokenInNotValid
+ return fmt.Errorf("invalid tokenIn: %w", err)
}
r.TokenIn = &tokenInCoin
}
if tokenOut := c.QueryParam("tokenOut"); tokenOut != "" {
tokenOutCoin, err := sdk.ParseCoinNormalized(tokenOut)
if err != nil {
- return ErrTokenOutNotValid
+ return fmt.Errorf("invalid tokenOut: %w", err)
}
r.TokenOut = &tokenOutCoin
}
r.TokenInDenom = c.QueryParam("tokenInDenom")
r.TokenOutDenom = c.QueryParam("tokenOutDenom")
return nil
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *GetQuoteRequest) UnmarshalHTTPRequest(c echo.Context) error { | |
| var err error | |
| r.SingleRoute, err = domain.ParseBooleanQueryParam(c, "singleRoute") | |
| if err != nil { | |
| return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()}) | |
| } | |
| r.ApplyExponents, err = domain.ParseBooleanQueryParam(c, "applyExponents") | |
| if err != nil { | |
| return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()}) | |
| } | |
| if tokenIn := c.QueryParam("tokenIn"); tokenIn != "" { | |
| tokenInCoin, err := sdk.ParseCoinNormalized(tokenIn) | |
| if err != nil { | |
| return ErrTokenInNotValid | |
| } | |
| r.TokenIn = &tokenInCoin | |
| } | |
| if tokenOut := c.QueryParam("tokenOut"); tokenOut != "" { | |
| tokenOutCoin, err := sdk.ParseCoinNormalized(tokenOut) | |
| if err != nil { | |
| return ErrTokenOutNotValid | |
| } | |
| r.TokenOut = &tokenOutCoin | |
| } | |
| r.TokenInDenom = c.QueryParam("tokenInDenom") | |
| r.TokenOutDenom = c.QueryParam("tokenOutDenom") | |
| return nil | |
| } | |
| func (r *GetQuoteRequest) UnmarshalHTTPRequest(c echo.Context) error { | |
| var err error | |
| r.SingleRoute, err = domain.ParseBooleanQueryParam(c, "singleRoute") | |
| if err != nil { | |
| return err | |
| } | |
| r.ApplyExponents, err = domain.ParseBooleanQueryParam(c, "applyExponents") | |
| if err != nil { | |
| return err | |
| } | |
| if tokenIn := c.QueryParam("tokenIn"); tokenIn != "" { | |
| tokenInCoin, err := sdk.ParseCoinNormalized(tokenIn) | |
| if err != nil { | |
| return fmt.Errorf("invalid tokenIn: %w", err) | |
| } | |
| r.TokenIn = &tokenInCoin | |
| } | |
| if tokenOut := c.QueryParam("tokenOut"); tokenOut != "" { | |
| tokenOutCoin, err := sdk.ParseCoinNormalized(tokenOut) | |
| if err != nil { | |
| return fmt.Errorf("invalid tokenOut: %w", err) | |
| } | |
| r.TokenOut = &tokenOutCoin | |
| } | |
| r.TokenInDenom = c.QueryParam("tokenInDenom") | |
| r.TokenOutDenom = c.QueryParam("tokenOutDenom") | |
| return nil | |
| } |
| func (s *RouterTestHelper) newRoutablePool(pool sqsdomain.PoolI, tokenOutDenom string, takerFee osmomath.Dec, cosmWasmConfig domain.CosmWasmPoolRouterConfig) domain.RoutablePool { | ||
| cosmWasmPoolsParams := pools.CosmWasmPoolsParams{ | ||
| Config: cosmWasmConfig, | ||
| ScalingFactorGetterCb: domain.UnsetScalingFactorGetterCb, | ||
| } | ||
| routablePool, err := pools.NewRoutablePool(pool, tokenOutDenom, takerFee, cosmWasmPoolsParams) | ||
|
|
||
| s.Require().NoError(err) | ||
|
|
||
| return routablePool | ||
| } |
There was a problem hiding this comment.
Address the linter issue.
The cosmWasmConfig parameter always receives a nil value. Consider removing the parameter if it is not needed or ensure it receives a valid value.
The linter issue needs to be addressed.
Tools
GitHub Check: Run linter
[failure] 44-44:
(*RouterTestHelper).newRoutablePool-cosmWasmConfigalways receivesdomain.CosmWasmPoolRouterConfig{}(nil) (unparam)
| }, | ||
| }, | ||
| }, | ||
| expectedStatusCode: http.StatusOK, | ||
| expectedResponse: s.MustReadFile("../../usecase/routertesting/parsing/quote_amount_in_response.json"), | ||
| expectedError: nil, | ||
| }, | ||
| { | ||
| name: "valid exact out request", | ||
| queryParams: map[string]string{ | ||
| "tokenOut": "1000ibc/498A0751C798A0D9A389AA3691123DADA57DAA4FE165D5C75894505B876BA6E4", | ||
| "tokenInDenom": "ibc/EA1D43981D5C9A1C4AAEA9C23BB1D4FA126BA9BC7020A25E0AE4AA841EA25DC5", | ||
| "singleRoute": "true", | ||
| "applyExponents": "true", | ||
| }, | ||
| handler: &routerdelivery.RouterHandler{ | ||
| TUsecase: &mocks.TokensUsecaseMock{ | ||
| IsValidChainDenomFunc: func(chainDenom string) bool { | ||
| return true | ||
| }, | ||
| }, | ||
| RUsecase: &mocks.RouterUsecaseMock{ | ||
| GetOptimalQuoteInGivenOutFunc: func(ctx context.Context, tokenIn sdk.Coin, tokenOutDenom string, opts ...domain.RouterOption) (domain.Quote, error) { | ||
| return s.NewExactAmountOutQuote(poolOne, poolTwo, poolThree), nil | ||
| }, | ||
| }, | ||
| }, | ||
| expectedStatusCode: http.StatusOK, | ||
| expectedResponse: s.MustReadFile("../../usecase/routertesting/parsing/quote_amount_out_response.json"), | ||
| expectedError: nil, | ||
| }, | ||
| { | ||
| name: "invalid swap method request", | ||
| queryParams: map[string]string{ | ||
| "tokenIn": "1000ust", | ||
| "tokenOut": "1000usdc", | ||
| "singleRoute": "true", | ||
| "applyExponents": "true", | ||
| }, | ||
| expectedStatusCode: http.StatusInternalServerError, | ||
| expectedResponse: `{"message": "swap method is invalid - must be either swap exact amount in or swap exact amount out"}`, | ||
| expectedError: routertypes.ErrSwapMethodNotValid, | ||
| }, | ||
| { | ||
| name: "invalid tokenIn format", | ||
| queryParams: map[string]string{ | ||
| "tokenIn": "invalid_denom", | ||
| "tokenOutDenom": "usdc", | ||
| "singleRoute": "true", | ||
| "applyExponents": "true", | ||
| }, | ||
| expectedStatusCode: http.StatusInternalServerError, | ||
| expectedResponse: `{"message": "tokenIn is invalid - must be in the format amountDenom"}`, | ||
| expectedError: routertypes.ErrTokenInNotValid, | ||
| }, | ||
| { | ||
| name: "invalid tokenOut format", | ||
| queryParams: map[string]string{ | ||
| "tokenInDenom": "ust", | ||
| "tokenOut": "invalid_denom", | ||
| "singleRoute": "true", | ||
| "applyExponents": "true", | ||
| }, | ||
| expectedStatusCode: http.StatusInternalServerError, | ||
| expectedResponse: `{"message": "tokenOut is invalid - must be in the format amountDenom"}`, | ||
| expectedError: routertypes.ErrTokenOutNotValid, | ||
| }, | ||
| } | ||
| for _, tc := range testcases { | ||
| s.Run(tc.name, func() { | ||
| e := echo.New() | ||
| req := httptest.NewRequest(echo.POST, "/", nil) | ||
| req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON) | ||
| q := req.URL.Query() | ||
| for k, v := range tc.queryParams { | ||
| q.Add(k, v) | ||
| } | ||
| req.URL.RawQuery = q.Encode() | ||
| rec := httptest.NewRecorder() | ||
| c := e.NewContext(req, rec) | ||
|
|
||
| err := tc.handler.GetOptimalQuote(c) | ||
|
|
||
| if tc.expectedError != nil { | ||
| s.Assert().Error(err) | ||
| s.Assert().Equal(tc.expectedError, err) | ||
| s.Assert().Equal(tc.expectedStatusCode, rec.Code) | ||
| s.Assert().JSONEq(tc.expectedResponse, rec.Body.String()) | ||
| return | ||
| } | ||
|
|
||
| s.Assert().NoError(err) | ||
| s.Assert().Equal(tc.expectedStatusCode, rec.Code) | ||
| s.Assert().JSONEq( | ||
| strings.TrimSpace(tc.expectedResponse), | ||
| strings.TrimSpace(rec.Body.String()), | ||
| ) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance test coverage and verify pool creation.
The function is well-structured and covers various scenarios. However, consider adding more edge cases and verifying the pool creation process to ensure comprehensive test coverage.
- Add more edge cases, such as invalid pool IDs and edge cases for token amounts.
- Verify the pool creation process to ensure pools are created correctly before running the tests.
|
While I still believe that merging suggestions introduced in this PR would be beneficial, however due working on more high priority tasks it would be hard to allocate time for further discussions, thus I am cllosing this for now. We can always get back to it in the future. cc @p0mvn |
This PR covers
TestGetOptimalQuoteInGivenOutwith tests.As a side effect
GetOptimalQuotemethod is broken is smaller parts to in order to be able to mock it. Such approach allows to tests smaller units indepently as well.I have tested indroduced changes with Unit tests and integration tests:
This PR will be merged is
DATA-276branch.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores