PR'ing some comments as I was reading through the getCandidateRoutes code #171
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
p0mvn
approved these changes
May 17, 2024
p0mvn
reviewed
May 17, 2024
Comment on lines
+19
to
+25
| // TODO: Build a better algorithm for finding routes. | ||
| // Right now we iterate over the etnire sorted list to try building routes. But most of the work is wasted | ||
| // in every iteration, as we have to think about pools that won't relate to the needed asset. | ||
| // instead we should have in router: | ||
| // * sortedPoolsByDenom map[string][]sqsdomain.PoolI. Where the return value is all pools that contain the denom, sorted. | ||
| // - Right now we have linear time iteration per route rather than N^2 by making every route get created in sorted order. | ||
| // - We can do similar here by actually making the value of the hashmap be a []struct{global sort index, sqsdomain pool} |
p0mvn
added a commit
that referenced
this pull request
May 17, 2024
Co-authored-by: Roman <[email protected]>
p0mvn
added a commit
that referenced
this pull request
May 17, 2024
* negligible improvement (#188) * negligible improvement * lint --------- Co-authored-by: Roman <[email protected]> * Some comments as I looked through this code (#171) Co-authored-by: Roman <[email protected]> --------- Co-authored-by: Dev Ojha <[email protected]>
cryptomatictrader
added a commit
that referenced
this pull request
May 24, 2024
* fix: pools with low liquidity being included in routes (#191) * fix: pools with low liquidity being included in routes * fix: linting issues for pricing route code * chore: update error messages * fix: remove using the cache for the pricing query * chore: add v25.x backport tag to mergify github action (#189) * fix: increase min liquidity for routing through pools (#193) * chore: update astroport code ID (#195) * feat: /config-private endpoint, mask otel in /config endpoint (#202) * feat: /config-private endpoint, mask otel in /config endpoint * lint * Update system/delivery/http/system_http_handler.go * docs: remove outdated context about ingest (#200) * docs: remove outdated context about ingest * updates * update config code ID for astroport PCL (#199) * changelog update (#206) * e2e test: foundation for getting desired testing tokens based on pool type, liquidity and volume (#198) * e2e test: foundation for getting desired testing tokens based on pool type, liquidity and volume * make astroport and transmuter helpers more useful * clean up * clean up * clean up * clean up pool types * clean up precomputed mappings * clean up token choice * liq filtering with pool types and more clean up * space * STABI-128 - Support Coingecko Prices * fix: DYDX spot price bug (#212) * changelog & sqsdomain * go mod fix * e2e: test for /tokens/metadata and router/routes (#207) * e2e test: foundation for getting desired testing tokens based on pool type, liquidity and volume * make astroport and transmuter helpers more useful * clean up * clean up * clean up * clean up pool types * clean up precomputed mappings * clean up token choice * liq filtering with pool types and more clean up * space * e2e: test for /tokens/metadata and router/routes * fetch expected number of routes from config * Changes based on code review, except the test related changes which will be checked in later * POC: Flight recording of slow requests (above 500ms) * Revert "POC: Flight recording of slow requests (above 500ms)" This reverts commit a7e9cef. * Added coingecko url and quote currency to default sqs config and test suite config * Updated tokens.json with coingecko_id in it * Added test cases to verify existence of coingecko_id in asset list, and verify the function of coingecko pricing source * Added test case to print chain denom in mainnet without coingecko support (no coingecko id in asset list) * Reuse the original tokens.json but update it with coingecko id so that it won't break test cases * Updated unsupported token count after adding coingeckto as fallback support * Verify unsupported token count for coingecko pricing source * Added the job for checking coingecko unsupported token list * Clean up chain pricing; add quote-based and spot-price based tests; add docs (#214) * Clean up chain pricing; add quote-based and spot-price based tests; add docs * updates * comments * changelog * Update tokens/delivery/http/tokens_delivery.go Added comment to explain how pricingSource parameter value will be interpreted. Co-authored-by: Roman <[email protected]> * Update tokens/usecase/pricing/coingecko/pricing_coingecko_test.go Removed unused function Co-authored-by: Roman <[email protected]> * Revision based on the comments from code review * Fixed lint commplaint * Complete candidate route test suite (#211) * e2e test: foundation for getting desired testing tokens based on pool type, liquidity and volume * make astroport and transmuter helpers more useful * clean up * clean up * clean up * clean up pool types * clean up precomputed mappings * clean up token choice * liq filtering with pool types and more clean up * space * e2e: test for /tokens/metadata and router/routes * fetch expected number of routes from config * more candidate route tests * Candidate routes test between every token and USDC * complete candidate route test suite * prevent false positives * comment * remove main.py * remove obsolete constants & avoid hardcoding * Update README to include coingecko documentation * demo monkey patch pricing getter mock (#218) * Use a mock for mocking the coingecko operation * Return 0 if coingecko id is empty in mock coingecko getter fn * Update tokens/usecase/pricing/coingecko/pricing_coingecko.go Co-authored-by: Roman <[email protected]> * negligible improvement (#188) * negligible improvement * lint --------- Co-authored-by: Roman <[email protected]> * Some comments as I looked through this code (#171) Co-authored-by: Roman <[email protected]> * Added test cases for checking the input validations in GetPrices in coingecko pricing source * Fixed linter complaint * Removed unused param, enhanced test cases for prices * Revert get_tokens_metadata pointing to PROD after local testing * Make returned value in coingecko mock consistent with real coingecko getter implementation * Filled in the non-exist coingecko id for wbtc in tokens.json * wbtc coingecko id is available, so one less unsupported token count in the assertion test * Pricing working test should check for both nil or zero price --------- Co-authored-by: PaddyMc <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Calvin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.