BE-686 | Implement FindCandidateRoutesInGivenOut #612
Conversation
Implements FindCandidateRoutesInGivenOut method for CandidateRouteSearcher. Additionally refactors some of the methods in RouterUsecase to highlight swap method for the operation.
WalkthroughThis pull request updates several method names and signatures across the codebase to clarify the directionality of token swaps. Many methods previously named generically are now suffixed with “OutGivenIn” or “InGivenOut” to indicate their operation modes explicitly. In addition, new methods have been introduced in both production and mock implementations to support reverse lookup of candidate routes and specialized pool preparations, all while retaining existing logic and error-handling flows. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant RH as RouterHandler
participant RU as RouterUseCase
participant CRF as CandidateRouteFinder
participant R as Route
C->>RH: Request Optimal Quote
RH->>RU: GetOptimalQuoteOutGivenIn(...)
RU->>CRF: FindCandidateRoutesOutGivenIn(...)
CRF-->>RU: Return candidate routes
RU->>R: PrepareResultPoolsOutGivenIn(...)
R-->>RU: Return processed pools
RU-->>RH: Return Quote
RH-->>C: Send Quote Response
sequenceDiagram
participant C as Client
participant RH as RouterHandler
participant RU as RouterUseCase
participant CRF as CandidateRouteFinder
C->>RH: Request Direct Quote (InGivenOut)
RH->>RU: GetCustomDirectQuoteInGivenOut(...)
RU->>CRF: FindCandidateRoutesInGivenOut(...)
CRF-->>RU: Return candidate routes
RU->>RU: Process routes via PrepareResultPoolsOutGivenIn(...)
RU-->>RH: Return Quote
RH-->>C: Send Quote Response
Possibly Related Issues
Possibly Related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (7)
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
router/usecase/candidate_routes.go (2)
241-241: Mismatch in doc comment.
The docstring says “FindCandidateRoutesOutGivenIn” but the function is named “FindCandidateRoutesInGivenOut,” which may cause confusion.Below is a suggested fix for the doc comment:
-// FindCandidateRoutesOutGivenIn implements domain.CandidateRouteFinder. +// FindCandidateRoutesInGivenOut implements domain.CandidateRouteFinder.
242-440: Consider reducing duplicated BFS logic.
The newFindCandidateRoutesInGivenOutmethod closely mirrors the logic inFindCandidateRoutesOutGivenIn, which could increase maintenance overhead. Extracting common BFS elements into a shared helper could reduce duplication and aid readability.router/usecase/optimized_routes.go (1)
215-332: Consider extracting common validation logic.The new
validateAndFilterRoutesInGivenOutfunction shares significant logic withvalidateAndFilterRoutesOutGivenIn. Consider extracting the common validation logic into a shared helper function to reduce code duplication.+func validateAndFilterRoutesCommon( + candidateRoutes []candidateRouteWrapper, + tokenA string, + tokenB string, + isOutGivenIn bool, + logger log.Logger, +) (ingesttypes.CandidateRoutes, error) { + var ( + filteredRoutes []ingesttypes.CandidateRoute + ) + + uniquePoolIDs := make(map[uint64]struct{}) + containsCanonicalOrderbook := false + + // ... common validation logic ... + + return ingesttypes.CandidateRoutes{ + Routes: filteredRoutes, + UniquePoolIDs: uniquePoolIDs, + ContainsCanonicalOrderbook: containsCanonicalOrderbook, + }, nil +}router/usecase/router_usecase.go (1)
436-436: Add documentation for renamed methods.The renamed methods lack documentation. Consider adding documentation similar to
GetOptimalQuoteOutGivenInto maintain consistency and improve code maintainability.Add documentation for:
GetCustomDirectQuoteOutGivenInGetCustomDirectQuoteMultiPoolOutGivenInAlso applies to: 470-470
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
domain/route_test.gois excluded by!**/*_test.gopools/usecase/pools_usecase_test.gois excluded by!**/*_test.goquotesimulator/quote_simulator_test.gois excluded by!**/*_test.gorouter/delivery/http/router_handler_test.gois excluded by!**/*_test.gorouter/usecase/candidate_routes_bench_test.gois excluded by!**/*_test.gorouter/usecase/candidate_routes_test.gois excluded by!**/*_test.gorouter/usecase/dynamic_splits_test.gois excluded by!**/*_test.gorouter/usecase/export_test.gois excluded by!**/*_test.gorouter/usecase/optimized_routes_test.gois excluded by!**/*_test.gorouter/usecase/route/route_test.gois excluded by!**/*_test.gorouter/usecase/router_usecase_test.gois excluded by!**/*_test.go
📒 Files selected for processing (16)
domain/candidate_routes.go(1 hunks)domain/mocks/candidate_route_finder_mock.go(1 hunks)domain/mocks/quote_simulator_mock.go(1 hunks)domain/mocks/route_mock.go(3 hunks)domain/mocks/router_usecase_mock.go(3 hunks)domain/mvc/router.go(1 hunks)domain/quote_simulator.go(1 hunks)domain/router.go(1 hunks)ingest/usecase/plugins/orderbook/fillbot/cyclic_arb.go(1 hunks)quotesimulator/quote_simulator.go(1 hunks)router/delivery/http/router_handler.go(3 hunks)router/usecase/candidate_routes.go(4 hunks)router/usecase/optimized_routes.go(4 hunks)router/usecase/quote_out_given_in.go(1 hunks)router/usecase/route/route.go(2 hunks)router/usecase/router_usecase.go(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run linter
- GitHub Check: build
- GitHub Check: Summary
🔇 Additional comments (24)
router/usecase/candidate_routes.go (3)
39-40: Good alignment with the new naming convention.
These lines introduce and document a function name consistent with the “OutGivenIn” concept, improving clarity about the directionality of the token swap logic.
80-80: Field assignment is consistent with the new direction.
SettingTokenInDenomtotokenIn.Denomaligns correctly with this route's “out given in” approach.
238-239: Returning filtered results is appropriate.
InvokingvalidateAndFilterRoutesOutGivenInensures post-processing of routes remains centralized, maintaining a clear separation of concerns.domain/quote_simulator.go (1)
11-16: Renaming is consistent with directionality.
Shifting fromSimulateQuotetoSimulateQuoteOutGivenInclarifies the method’s purpose related to “out given in” quotes, adhering to the revised naming standard.domain/mocks/quote_simulator_mock.go (1)
14-15: Mock method name updated correctly.
The rename ensures consistency with the interface’sSimulateQuoteOutGivenInmethod, maintaining clarity in tests.domain/mocks/candidate_route_finder_mock.go (2)
16-19: LGTM! The renamed method maintains interface compatibility.The implementation correctly returns the mock's predefined values.
21-24: LGTM! The new method follows the same pattern.The implementation is consistent with the existing mock pattern.
quotesimulator/quote_simulator.go (1)
33-34: LGTM! Method rename improves clarity.The rename better reflects the direction of the token swap operation while maintaining the existing functionality.
ingest/usecase/plugins/orderbook/fillbot/cyclic_arb.go (1)
22-22: LGTM! Method call updated for consistency.The rename aligns with the broader refactoring pattern while maintaining the existing functionality.
domain/candidate_routes.go (1)
68-71: LGTM! Method rename improves clarity.The rename better reflects the direction of the token swap operation.
domain/mocks/route_mock.go (3)
13-14: LGTM! Good addition of the reverse calculation function.The addition of
CalculateTokenInByTokenOutFunccomplements the existingCalculateTokenOutByTokenInFunc, providing symmetrical functionality for both swap directions.
19-20: LGTM! Clear separation of pool preparation functions.The split into
PrepareResultPoolsExactAmountInFuncandPrepareResultPoolsExactAmountOutFuncimproves clarity by explicitly indicating the direction of the swap operation.
81-88: LGTM! Consistent method renaming.The renaming of
PrepareResultPoolstoPrepareResultPoolsOutGivenInaligns with the broader refactoring effort to make token swap directionality explicit.router/usecase/quote_out_given_in.go (1)
83-83: LGTM! Method call updated to match new interface.The method call has been updated from
PrepareResultPoolstoPrepareResultPoolsOutGivenIn, maintaining consistency with the interface changes.router/usecase/route/route.go (1)
44-57: LGTM! Method renamed for clarity while preserving functionality.The renaming from
PrepareResultPoolstoPrepareResultPoolsOutGivenInbetter describes the method's purpose. The implementation maintains proper error handling and metrics tracking.domain/mvc/router.go (2)
62-64: LGTM! Method renamed to clarify swap direction.The renaming from
GetOptimalQuotetoGetOptimalQuoteOutGivenInbetter indicates that this method handles exact amount in swaps.
68-74: LGTM! Consistent method renaming for direct quotes.The renaming of
GetCustomDirectQuoteandGetCustomDirectQuoteMultiPoolto includeOutGivenInsuffix maintains consistency with the new naming convention while preserving functionality.domain/mocks/router_usecase_mock.go (1)
21-27: LGTM! Method renaming improves clarity.The renaming of methods to include directional suffixes (
OutGivenIn,InGivenOut) makes the token swap direction more explicit and aligns with the PR objectives.domain/router.go (1)
41-49: LGTM! Method renaming maintains documentation clarity.The renaming of
PrepareResultPoolstoPrepareResultPoolsOutGivenInis consistent with the codebase-wide refactoring while preserving the detailed documentation of the method's functionality.router/usecase/optimized_routes.go (1)
23-83: LGTM! Method renaming maintains functionality.The renaming of
estimateAndRankSingleRouteQuotetoestimateAndRankSingleRouteQuoteOutGivenInimproves clarity while preserving the existing logic for quote estimation and ranking.router/delivery/http/router_handler.go (1)
131-131: LGTM! Method calls updated consistently.The updates to method calls (
GetOptimalQuoteOutGivenIn,SimulateQuoteOutGivenIn,GetCustomDirectQuoteMultiPoolOutGivenIn) are consistent with the interface changes and maintain the existing functionality.Also applies to: 159-159, 235-235
router/usecase/router_usecase.go (3)
79-91: LGTM! Method renames improve API clarity.The consistent renaming pattern with "OutGivenIn" and "InGivenOut" suffixes makes the swap directionality explicit in the API. This aligns well with the PR objectives and improves code readability.
Also applies to: 436-437, 470-471
192-218: LGTM! Implementation handles orderbook pools correctly.The implementation correctly:
- Updates the method call to use the renamed method
- Disables cache to prevent interference with the main API
- Filters out orderbook pools that don't support MsgSwapExactAmountOut
472-483: LGTM! Comprehensive input validation.The error handling thoroughly validates inputs with clear error messages:
- Checks for empty pool IDs
- Checks for empty token out denoms
- Ensures matching lengths between pool IDs and token out denoms
| @@ -59,19 +59,19 @@ type SimpleRouterUsecase interface { | |||
| type RouterUsecase interface { | |||
| SimpleRouterUsecase | |||
|
|
|||
| // GetOptimalQuote returns the optimal quote for the given tokenIn and tokenOutDenom. | |||
| GetOptimalQuote(ctx context.Context, tokenIn sdk.Coin, tokenOutDenom string, opts ...domain.RouterOption) (domain.Quote, error) | |||
| // GetOptimalQuoteOutGivenIn returns the optimal quote for the given token swap method exact amount in. | |||
There was a problem hiding this comment.
Note for reviewer: No actual changes to those methods but renaming since we're preparing for introducing alternative methods for InGivenOut swap methods, for example GetCustomDirectQuoteInGivenOut. Renaming allows to use consistent names, for example GetOptimalQuote became GetOptimalQuoteOutGivenIn aligning with existing GetOptimalQuoteOutGivenIn.
| // CONTRACT: | ||
| // - Only direct (non-split) quotes are supported. | ||
| // Retursn error if: | ||
| // - Simulator address does not have enough funds to pay for the quote. | ||
| SimulateQuote(ctx context.Context, quote Quote, slippageToleranceMultiplier osmomath.Dec, simulatorAddress string) TxFeeInfo | ||
| SimulateQuoteOutGivenIn(ctx context.Context, quote Quote, slippageToleranceMultiplier osmomath.Dec, simulatorAddress string) TxFeeInfo |
There was a problem hiding this comment.
Note for reiviewer: renaming in preparation introducing SimulateQuoteInGivenOut.
| // from each pool in the route, | ||
| // leaving only the data needed by client | ||
| // Runs the quote logic one final time to compute the effective spot price. | ||
| // Note that it mutates the route. | ||
| // Computes the spot price of the route. | ||
| // Returns the spot price before swap and effective spot price. | ||
| // The token in is the base token and the token out is the quote token. | ||
| PrepareResultPools(ctx context.Context, tokenIn sdk.Coin, logger log.Logger) ([]RoutablePool, osmomath.Dec, osmomath.Dec, error) | ||
| PrepareResultPoolsOutGivenIn(ctx context.Context, tokenIn sdk.Coin, logger log.Logger) ([]RoutablePool, osmomath.Dec, osmomath.Dec, error) |
There was a problem hiding this comment.
Note for reviewer: Renaming in preparation introducing PrepareResultPoolsInGivenOut
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/usecase/optimized_routes.go (1)
215-332: Consider refactoring to reduce code duplication.While the implementation is correct, there's significant code duplication between
validateAndFilterRoutesInGivenOutandvalidateAndFilterRoutesOutGivenIn. Consider extracting common validation logic into a shared helper function.Here's a suggested approach:
+ type validationConfig struct { + routes []candidateRouteWrapper + tokenInDenom string + tokenOutDenom string + isInGivenOut bool + logger log.Logger + } + + func validateAndFilterRoutes(config validationConfig) (ingesttypes.CandidateRoutes, error) { + // Common validation logic here + // Use config.isInGivenOut to determine token flow direction + } + - func validateAndFilterRoutesOutGivenIn(candidateRoutes []candidateRouteWrapper, tokenInDenom string, logger log.Logger) (ingesttypes.CandidateRoutes, error) { + func validateAndFilterRoutesOutGivenIn(candidateRoutes []candidateRouteWrapper, tokenInDenom string, logger log.Logger) (ingesttypes.CandidateRoutes, error) { + return validateAndFilterRoutes(validationConfig{ + routes: candidateRoutes, + tokenInDenom: tokenInDenom, + isInGivenOut: false, + logger: logger, + }) } - func validateAndFilterRoutesInGivenOut(candidateRoutes []candidateRouteWrapper, tokenOutDenom string, logger log.Logger) (ingesttypes.CandidateRoutes, error) { + func validateAndFilterRoutesInGivenOut(candidateRoutes []candidateRouteWrapper, tokenOutDenom string, logger log.Logger) (ingesttypes.CandidateRoutes, error) { + return validateAndFilterRoutes(validationConfig{ + routes: candidateRoutes, + tokenOutDenom: tokenOutDenom, + isInGivenOut: true, + logger: logger, + }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router/usecase/export_test.gois excluded by!**/*_test.gorouter/usecase/optimized_routes_test.gois excluded by!**/*_test.go
📒 Files selected for processing (1)
router/usecase/optimized_routes.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
router/usecase/optimized_routes.go (2)
23-23: LGTM! Clear and descriptive method renaming.The method name now explicitly indicates that it estimates routes for swapping a given input token for an output token, improving code clarity.
85-93: LGTM! Enhanced method documentation and data structure.The method name and documentation now clearly indicate the validation flow for out-given-in routes. The addition of
TokenInDenomfield improves token flow tracking.Also applies to: 197-197
| @@ -45,7 +45,7 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
| steps: | |||
| - name: Clone Repository | |||
| uses: actions/checkout@master | |||
| uses: actions/checkout@v4 | |||
There was a problem hiding this comment.
Note for reviewer: fixes build "failed because it uses a deprecated version of actions/cache: v4.0.2." error
|
| @@ -54,7 +54,7 @@ jobs: | |||
| name: code-coverage-report | |||
| # path: app | |||
| - name: Analyze with SonarCloud | |||
| uses: sonarsource/sonarcloud-github-action@master | |||
| uses: sonarsource/sonarqube-scan-action@v4 | |||
There was a problem hiding this comment.
Note for reviewer: sonarsource/sonarcloud-github-action@master action seems to be deprecated and no longer works, alternative sonarsource/sonarqube-scan-action@v4 fixed the issue.
| uniquePoolIDs: map[uint64]struct{}{ | ||
| 1066: {}, | ||
| 1077: {}, | ||
| 1078: {}, | ||
| 1079: {}, | ||
| 1081: {}, | ||
| 1094: {}, | ||
| 1110: {}, | ||
| 1133: {}, | ||
| 1134: {}, | ||
| 1135: {}, | ||
| 1159: {}, | ||
| 1189: {}, | ||
| 1205: {}, | ||
| 1220: {}, | ||
| 1221: {}, | ||
| 1261: {}, | ||
| 1263: {}, | ||
| 1265: {}, | ||
| 1279: {}, | ||
| 1281: {}, | ||
| 1399: {}, | ||
| 1400: {}, | ||
| 1464: {}, | ||
| 1477: {}, | ||
| 1895: {}, | ||
| }, |
There was a problem hiding this comment.
I think we should avoid hardcoding specific pool IDs. Not sure what exactly this assertion accomplishes
Otherwise, if we regen the state, the tests will break, requiring us to spend time modifying.
There was a problem hiding this comment.
The goal of using specific pool IDs is to ensure that computed candidate routes includes those expected pools.
When implementing InGivenOut I broke the algorithm and it returned diferent routes than OutGiveIn but tests did not catched this case.
I agree it's not ideal, in ideal situation we should be testing against small hardcoded set of routes, but since we're testing against huge state file it produces such drawbacks, but I think it's better than leaving as it is.
Implements FindCandidateRoutesInGivenOut method for
CandidateRouteSearcher. Covers it with bench and unit tests.Implementation is based on
FindCandidateRoutesOutGivenInby inverting resulting denominations. Test cases were adjusted to be more strict and to take into account actual specific pools returned and number of routes for each test case to ensure stability for each swap method.Bench results:
Additionally refactors some of the method names in RouterUsecase to highlight swap method for the operation which results in all that noise in the PR.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor