feat/refactor/test: introduce dynamic min liquidity into pricing; fix e2e flakiness & alloyed bug #376
Conversation
|
Warning Review failedThe pull request is closed. WalkthroughThe changes introduce enhanced handling of token metadata and pool denomination metadata, improve pricing logic, and integrate new routing capabilities. Key modifications include the introduction of the Alloyed Transmuter Pools by Osmosis, asynchronous price updates, and updated initialization procedures for several use cases and routers. Additionally, mock implementations and new functions were added to streamline testing and functionality verification. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HTTPRouter as HTTP Router
participant RouterUsecase as Router Usecase
participant TokensUsecase as Tokens Usecase
participant MetadataHolder as Token Metadata Holder
User->>HTTPRouter: Request Optimal Quote
HTTPRouter->>RouterUsecase: GetOptimalQuote
RouterUsecase->>TokensUsecase: Fetch Token Metadata
TokensUsecase->>MetadataHolder: ClearPoolDenomMetadata
MetadataHolder->>TokensUsecase: Metadata Cleared
TokensUsecase->>RouterUsecase: Metadata Retrieved
RouterUsecase->>HTTPRouter: Optimal Quote
HTTPRouter->>User: Return Optimal Quote
Poem
Tip You can customize the tone of the comments in your PRsSpecify the tone of the comments in your PRs by configuring the 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 Configration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
1 similar comment
Actions performedReview triggered.
|
![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
Outside diff range and nitpick comments (4)
domain/mvc/router.go (1)
85-90: Add method documentation.The added method
GetMinPoolLiquidityCapFiltershould have a clear and concise documentation comment explaining its purpose and usage.// GetMinPoolLiquidityCapFilter returns the minimum pool liquidity cap filter for the given token pair. // It is used to filter out pools with liquidity less than the output of this function. // Returns error if one of the denom metadata is not found or if the filter is not found for the given denoms. GetMinPoolLiquidityCapFilter(tokenInDenom, tokenOutDenom string) (uint64, error)domain/mvc/tokens.go (1)
87-89: Add method documentation.The added method
ClearPoolDenomMetadatashould have a clear and concise documentation comment explaining its purpose and usage.// ClearPoolDenomMetadata clears all pool denom metadata. // WARNING: use with caution, this will clear all pool denom metadata. ClearPoolDenomMetadata()docs/architecture/ingest.md (1)
72-77: Fix typographical errors.There are a few typographical errors in the added note. Here's the corrected version:
- Note, that we recompute pricing logic twice for the first block ingested. First time it is done - syncronously to avoid proceeding before prices are computed. At that point the pool liquidity pricing - is not enabled yet since we have no pricing data. Therefore, the prices are compouted via suboptimal routes. - As a result, once the prices for all tokens are computed the first time, we trigger the pricing worker - asyncronously for all tokens second time. + Note that we recompute pricing logic twice for the first block ingested. The first time it is done + synchronously to avoid proceeding before prices are computed. At that point, the pool liquidity pricing + is not enabled yet since we have no pricing data. Therefore, the prices are computed via suboptimal routes. + As a result, once the prices for all tokens are computed the first time, we trigger the pricing worker + asynchronously for all tokens a second time.Tools
LanguageTool
[typographical] ~73-~73: It appears that a comma is missing.
Context: ...ing before prices are computed. At that point the pool liquidity pricing is not enabl...(DURING_THAT_TIME_COMMA)
router/usecase/optimized_routes_test.go (1)
563-563: Ensure consistency in constant naming.The constant
usdtOsmoExpectedRoutesHighLiqshould follow a consistent naming convention.Consider renaming it to
expectedRoutesHighLiqUSDTOSMOfor better readability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (23)
- Makefile (1 hunks)
- app/sidecar_query_server.go (2 hunks)
- docs/architecture/ingest.md (1 hunks)
- domain/mocks/token_metadata_holder_mock.go (1 hunks)
- domain/mvc/router.go (1 hunks)
- domain/mvc/tokens.go (3 hunks)
- ingest/usecase/ingest_usecase.go (1 hunks)
- router/delivery/http/export_test.go (1 hunks)
- router/delivery/http/router_handler.go (3 hunks)
- router/delivery/http/router_handler_test.go (2 hunks)
- router/usecase/candidate_routes.go (2 hunks)
- router/usecase/optimized_routes_test.go (4 hunks)
- router/usecase/router_usecase.go (6 hunks)
- router/usecase/router_usecase_test.go (6 hunks)
- router/usecase/routertesting/parsing/mainnet_pools.go (3 hunks)
- router/usecase/routertesting/parsing/pool_denom_metadata.json (1 hunks)
- router/usecase/routertesting/suite.go (7 hunks)
- tests/test_pools.py (1 hunks)
- tokens/delivery/http/tokens_delivery.go (1 hunks)
- tokens/usecase/pricing/coingecko/pricing_coingecko_test.go (1 hunks)
- tokens/usecase/pricing/worker/pricing_worker_test.go (1 hunks)
- tokens/usecase/tokens_usecase.go (1 hunks)
- tokens/usecase/tokens_usecase_test.go (3 hunks)
Files not summarized due to errors (1)
- router/usecase/routertesting/parsing/pool_denom_metadata.json: Error: Message exceeds token limit
Files skipped from review due to trivial changes (3)
- Makefile
- router/delivery/http/router_handler_test.go
- tokens/usecase/pricing/worker/pricing_worker_test.go
Additional context used
LanguageTool
docs/architecture/ingest.md
[typographical] ~73-~73: It appears that a comma is missing.
Context: ...ing before prices are computed. At that point the pool liquidity pricing is not enabl...(DURING_THAT_TIME_COMMA)
Additional comments not posted (35)
router/delivery/http/export_test.go (1)
Line range hint
1-7:
LGTM!The function is a simple wrapper and there are no issues with the code.
domain/mocks/token_metadata_holder_mock.go (1)
1-15: LGTM!The mock implementation is straightforward and there are no issues with the code.
tests/test_pools.py (1)
Line range hint
1-52:
LGTM!The test class and methods are well-structured and cover various scenarios. There are no issues with the code.
tokens/usecase/pricing/coingecko/pricing_coingecko_test.go (1)
Line range hint
1-113:
LGTM!The test methods are well-structured and cover various scenarios. There are no issues with the code.
app/sidecar_query_server.go (2)
114-114: Ensure proper initialization ofpricingSimpleRouterUsecase.The initialization of
pricingSimpleRouterUsecasenow includestokensUseCase. Ensure thattokensUseCaseis properly initialized before it is passed toNewRouterUsecase.Verification successful
Ensure proper initialization of
tokensUseCasebeforepricingSimpleRouterUsecase.The
tokensUseCaseis initialized before it is passed toNewRouterUsecase, confirming the correct sequence.
app/sidecar_query_server.go:tokensUseCaseis properly initialized beforepricingSimpleRouterUsecase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of `tokensUseCase` before it is passed to `NewRouterUsecase`. # Test: Ensure proper initialization of `tokensUseCase`. Expect: `tokensUseCase` is initialized before `pricingSimpleRouterUsecase`. rg --type go 'tokensUseCase := tokensUseCase.NewTokensUsecase'Length of output: 168
105-105: Ensure proper initialization ofrouterUsecase.The initialization of
routerUsecasenow includestokensUseCase. Ensure thattokensUseCaseis properly initialized before it is passed toNewRouterUsecase.Verification successful
Initialization of
tokensUseCaseis correct.The
tokensUseCaseis properly initialized before it is passed toNewRouterUsecaseinapp/sidecar_query_server.go.
tokensUseCaseis initialized at line 105.routerUsecaseis initialized aftertokensUseCase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of `tokensUseCase` before it is passed to `NewRouterUsecase`. # Test: Ensure proper initialization of `tokensUseCase`. Expect: `tokensUseCase` is initialized before `routerUsecase`. rg --type go 'tokensUseCase := tokensUseCase.NewTokensUsecase'Length of output: 168
Script:
#!/bin/bash # Description: Verify the order of initialization for `tokensUseCase` and `routerUsecase` in the `app/sidecar_query_server.go` file. # Print lines around the initialization of `tokensUseCase` and `routerUsecase` to check their order. rg --type go -A 5 -B 5 'tokensUseCase := tokensUseCase.NewTokensUsecase' app/sidecar_query_server.go rg --type go -A 5 -B 5 'routerUsecase := routerUseCase.NewRouterUsecase' app/sidecar_query_server.goLength of output: 1210
router/usecase/routertesting/parsing/mainnet_pools.go (2)
123-133: LGTM! Ensure unit tests cover the new function.The
StorePoolDenomMetaDatafunction is consistent with existing functions and handles file operations and JSON marshaling correctly.If not already covered, consider adding unit tests for this function.
259-272: LGTM! Ensure unit tests cover the new function.The
ReadPoolDenomsMetaDatafunction is consistent with existing functions and handles file operations and JSON unmarshaling correctly.If not already covered, consider adding unit tests for this function.
tokens/delivery/http/tokens_delivery.go (1)
324-330: LGTM! Ensure unit tests cover the new functionality.The changes to
StoreTokensStateInFilescorrectly handle storing pool denom metadata.If not already covered, consider adding unit tests for this functionality.
router/delivery/http/router_handler.go (1)
112-112: LGTM!The changes to
GetOptimalQuotecorrectly remove theGetMinPoolLiquidityCapFiltermethod and simplify the code.ingest/usecase/ingest_usecase.go (2)
153-157: LGTM! Verify the correctness of the function call.The comments and the call to
UpdatePricesAsyncfor the second time are necessary to ensure accurate pricing. Ensure that the function works as expected.Verification successful
LGTM! Verify the correctness of the function call.
The function
UpdatePricesAsyncis defined intokens/usecase/pricing/worker/pricing_worker.goand is tested intokens/usecase/pricing/worker/pricing_worker_test.go. Additionally, it is referenced indomain/pricing.go. The presence of tests suggests that the function is being validated for correctness.
- Function Definition:
tokens/usecase/pricing/worker/pricing_worker.go- Tests:
tokens/usecase/pricing/worker/pricing_worker_test.go- Interface/Documentation:
domain/pricing.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `UpdatePricesAsync` function call. # Test: Search for the function definition. Expect: The function should be defined and correctly implemented. rg --type go 'func UpdatePricesAsync'Length of output: 39
Script:
#!/bin/bash # Description: Verify the correctness of the `UpdatePricesAsync` function call. # Test 1: Search for the method definition within class or interface definitions. ast-grep --lang go --pattern 'type $_ interface { $$$ UpdatePricesAsync($_) $$$ }' ast-grep --lang go --pattern 'type $_ struct { $$$ UpdatePricesAsync func($_) $$$ }' # Test 2: Search for the method in import statements. rg --type go 'UpdatePricesAsync'Length of output: 1267
150-152: LGTM! Verify the correctness of the function call.The introduction of
UpdatePricesSyncto pre-compute prices for all tokens synchronously during the first block is a good addition. Ensure that the function works as expected.Verification successful
The
UpdatePricesSyncfunction call is correctly implemented and used within the codebase.The function is defined in
tokens/usecase/pricing/worker/pricing_worker.goand is referenced appropriately in multiple locations, includingingest/usecase/ingest_usecase.go.
tokens/usecase/pricing/worker/pricing_worker.go: Function definition and callingest/usecase/ingest_usecase.go: Function calldomain/pricing.go: Interface definition and commentsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `UpdatePricesSync` function call. # Test: Search for the function definition. Expect: The function should be defined and correctly implemented. rg --type go 'func UpdatePricesSync'Length of output: 38
Script:
#!/bin/bash # Search for the definition of the UpdatePricesSync method within the entire codebase ast-grep --lang go --pattern 'func ($_).UpdatePricesSync($$$) { $$$ }'Length of output: 72
Script:
#!/bin/bash # Search for any references to UpdatePricesSync within the entire codebase to gather more context rg 'UpdatePricesSync'Length of output: 597
router/usecase/routertesting/suite.go (4)
45-47: LGTM!The addition of
PoolDenomsMetaDatato theMockMainnetStatestruct is necessary for the new dynamic minimum liquidity configuration.
58-63: LGTM!The addition of
poolDenomsMetaDataFileNameis necessary for the new dynamic minimum liquidity configuration.
Line range hint
338-350: LGTM!The addition of the step to read pool denomination metadata in the
SetupMainnetStatefunction is necessary for the new dynamic minimum liquidity configuration.
392-397: LGTM!The modification to update pool denomination metadata in the
SetupRouterAndPoolsUsecasefunction is necessary for the new dynamic minimum liquidity configuration.tokens/usecase/tokens_usecase.go (2)
134-139: LGTM!The addition of the
ClearPoolDenomMetadatamethod is necessary for the new dynamic minimum liquidity configuration. The warning comment is a good reminder to use this method with caution.
Line range hint
393-395: LGTM!The modification to update pool denomination metadata in the
UpdatePoolDenomMetadatamethod is necessary for the new dynamic minimum liquidity configuration.tokens/usecase/tokens_usecase_test.go (2)
364-365: LGTM!The addition of the step to clear pool denomination metadata in the
TestPoolDenomMetadatatest is necessary to ensure a clean state for the test.
526-527: LGTM!The addition of the step to clear pool denomination metadata in the
TestGetMinPoolLiquidityCaptest is necessary to ensure a clean state for the test.router/usecase/optimized_routes_test.go (7)
582-582: Verify the expected routes count.Ensure that the expected routes count for
usdt for umeeis accurate and based on the latest liquidity data.
590-590: Verify the expected routes count.Ensure that the expected routes count for
uosmo for uionis accurate and based on the latest liquidity data.
606-606: Verify the expected routes count.Ensure that the expected routes count for
uakt for umeeis accurate and based on the latest liquidity data.
693-695: Ensure mock initialization is correct.Verify that the
tokenMetaDataHolderMockinitialization is correct and aligned with the latest mock implementation.
Line range hint
45-45: LGTM!The test cases for
TestGetBestSplitRoutesQuoteare well-structured and cover various liquidity scenarios.
Line range hint
234-234: LGTM!The test cases for
TestValidateAndFilterRoutesare comprehensive and cover various validation and filtering scenarios.
693-693: LGTM!The test cases for
TestGetCustomQuote_GetCustomDirectQuote_Mainnet_UOSMOUIONare well-structured and cover custom quotes for UOSMO to UION.router/usecase/router_usecase.go (5)
91-95: LGTM!The initialization of the router use case with the new
tokenMetadataHolderparameter is correct.
149-154: LGTM!The retrieval and application of the dynamic minimum pool liquidity cap is correctly implemented.
230-235: LGTM!The retrieval and application of the dynamic minimum pool liquidity cap is correctly implemented.
768-789: LGTM!The function
GetMinPoolLiquidityCapFiltercorrectly retrieves and applies the minimum pool liquidity cap filter based on the dynamic configuration.
Line range hint
691-691: LGTM!The function
handleCandidateRoutescorrectly handles candidate routes with the new dynamic minimum pool liquidity cap.router/usecase/router_usecase_test.go (2)
243-245: Verify the usage ofTokenMetadataHolderMock.The
TokenMetadataHolderMockis instantiated but not actively used in the function. Ensure it is correctly integrated and serves its intended purpose.
1188-1270: LGTM! Ensure comprehensive test coverage.The new test function
TestGetMinPoolLiquidityCapFilteris comprehensive and covers various scenarios. Ensure that all edge cases are included.router/usecase/routertesting/parsing/pool_denom_metadata.json (1)
1-1: LGTM! Verify the accuracy of the data.The JSON structure and syntax are correct. Ensure that the values for
total_liquidity,total_liquidity_cap, andpriceare accurate and meaningful.
* fix alloyed prices breakage with candidate route optimization * update docs * unit test and docs * clean up * clean up * remove test
| @@ -54,6 +54,34 @@ One caveat on utilizing cw2 information is that there is no uniqueness check for | |||
| `crates.io:transmuter` `<3.0.0`: requires no additional information. | |||
| `crates.io:transmuter` `>=3.0.0`: requires alloyed asset denom and normalization factors for each asset. | |||
|
|
|||
| ### Alloyed | |||
There was a problem hiding this comment.
Note to reviewer:
Upon regenerating mainnet test state, another bug stemming from the treatment of alloyed denoms resurfaced.
It stemmed from the inconsistency in handling the alloyed LP share.
As an outcome, I defined the invariants and fixed the tests in this PR.
#377 was merged here to unblock the CI pipeline.
For ease of review, I recommend starting from #377 and then coming back to reviewing the remaining work here.
| // Total unsupported tokens as of June 30 2024 | ||
| s.Require().Equal(154, unsupportedCounter) | ||
| // Total unsupported tokens as of July 7 2024 | ||
| s.Require().Equal(158, unsupportedCounter) |
There was a problem hiding this comment.
Note: this seems to stem from new token listings
| # This is the max error tolerance of 7% that we allow. | ||
| # Arbitrarily hand-picked to avoid flakiness. | ||
| error_tolerance = 0.07 | ||
| # At a higher amount in, the volatility is much higher, leading to | ||
| # flakiness. Therefore, we increase the error tolerance to 13%. | ||
| # The values are arbitrarily hand-picke and can be adjusted if necessary. | ||
| # This seems to be especially relevant for the Astroport PCL pools. | ||
| if amount_in > 10_000: | ||
| error_tolerance = 0.10 | ||
| elif amount_in > 30_000_000_000: | ||
| error_tolerance = 0.13 | ||
| elif amount_in > 60_000_000_000: | ||
| error_tolerance = 0.16 | ||
| # Choosse the error tolerance based on amount in swapped. | ||
| error_tolerance = choose_error_tolerance(amount_in) |
There was a problem hiding this comment.
Note: all changes in this test file are drive-by to fix flakiness stemming from high-value amounts. There was a code bug that failed to correctly set the dynamic error tolerance based on the amounts
| if pool_id == skip_alloyed_pool_id: | ||
| pytest.skip("Skipping alloyed pool since it has flakiness on Numia side") |
There was a problem hiding this comment.
Note: confirmed on Slack with Numia
| { | ||
| MinTokensCap: 250000, | ||
| FilterValue: 15000, | ||
| }, | ||
| { | ||
| MinTokensCap: 10000, | ||
| FilterValue: 1000, | ||
| }, | ||
| { | ||
| MinTokensCap: 1000, | ||
| FilterValue: 10, | ||
| }, | ||
| { | ||
| MinTokensCap: 1, | ||
| FilterValue: 1, | ||
| }, |
There was a problem hiding this comment.
Note: extended our test configuration per the upcoming default on mainnet
| @@ -29,118 +27,6 @@ func TestRouterHandlerSuite(t *testing.T) { | |||
| suite.Run(t, new(RouterHandlerSuite)) | |||
| } | |||
|
|
|||
| func (s *RouterHandlerSuite) TestGetMinPoolLiquidityCapFilter() { | |||
There was a problem hiding this comment.
Note: this was simply moved to router/usecase/router_usecase_test.go
| @@ -1179,6 +1185,90 @@ func (s *RouterTestSuite) TestCutRoutesForSplits() { | |||
| } | |||
| } | |||
|
|
|||
| func (s *RouterTestSuite) TestGetMinPoolLiquidityCapFilter() { | |||
There was a problem hiding this comment.
Note: this was simply moved from router/delivery/http/router_handler_test.go
| dynamicMinPoolLiquidityCap, err := r.tokenMetadataHolder.GetMinPoolLiquidityCap(tokenIn.Denom, tokenOutDenom) | ||
| if err == nil { | ||
| // Set the dynamic min pool liquidity cap only if there is no error retrieving it. | ||
| // Oterwise, use default. | ||
| options.MinPoolLiquidityCap = r.ConvertMinTokensPoolLiquidityCapToFilter(dynamicMinPoolLiquidityCap) | ||
| } | ||
|
|
||
| // If this is pricing worker precomputation, we need to be able to call this as | ||
| // some pools have TVL incorrectly calculated as zero. For example, BRNCH / STRDST (1288). | ||
| // As a result, they are incorrectly excluded despite having appropriate liquidity. | ||
| // So we want to calculate price, but we never cache routes for pricing the are below the minPoolLiquidityCap value, as these are returned to users. | ||
|
|
||
| // Compute candidate routes. | ||
| candidateRoutes, err := GetCandidateRoutesNew(r.routerRepository.GetCandidateRouteSearchData(), tokenIn, tokenOutDenom, options.MaxRoutes, options.MaxPoolsPerRoute, r.logger) | ||
| candidateRoutes, err := GetCandidateRoutesNew(r.routerRepository.GetCandidateRouteSearchData(), tokenIn, tokenOutDenom, options.MaxRoutes, options.MaxPoolsPerRoute, options.MinPoolLiquidityCap, r.logger) |
There was a problem hiding this comment.
Note to reviewer: the core of the change is adding this logic that enabled the dynamic min liquidity into pricing.
The rest is either reshuffling prior methods around or fixing bugs as a drive-by.
p0mvn
changed the title
…dynamic-min-liquidity-into
p0mvn
deleted the
roman/data-253-candidaterouteopt-introduce-dynamic-min-liquidity-into
branch
Closes: https://linear.app/osmosis/issue/DATA-253/[candidaterouteopt]-introduce-dynamic-min-liquidity-into-pricing
Core Change
This PR introduces a shared dynamic min liquidity config to the pricing logic.
In integration tests we started getting pool liquidity capitalization test failures. Those stemmed from using low liquidity pools in pricing routes for tokens such as EVMOS. This feature has resolved the test failures.
The core of the change can be found near this comment. The rest of the diff is either reshufling older methods around to enable the integration or fixing drive-by bugs to unblock CI.
Drive-by Bug Fixes
Upon debugging more from the re-generated mainnet state, another bug was found due to the "alloyed" denoms. That bug started failing the tests in this PR, blocking merge.
The bug was fixed in a separate PR here. Detailed context is left in its description and documentation. I recommend reviewing it separately to avoid getting overwhelmed.
Scope
disableMinLiquidityCapFallbackandforceDefaultMinLiquidityCapquery params from/quotesince they have not proven to be useful and only add complexityTests
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests