fix: DYDX spot price bug #212
Merged
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
commented
May 10, 2024
| spotPrice, err := r.poolsUsecase.GetPoolSpotPrice(ctx, poolID, poolTakerFee, baseAsset, quoteAsset) | ||
| spotPrice, err := r.poolsUsecase.GetPoolSpotPrice(ctx, poolID, poolTakerFee, quoteAsset, baseAsset) |
There was a problem hiding this comment.
Note: This is a drive-by change. We were negating this in pricing_chain.go so it did not result in any bug
p0mvn
commented
May 10, 2024
Comment on lines
-224
to
-225
| } else { | ||
| chainPrice = osmomath.OneBigDec().QuoMut(chainPrice) |
There was a problem hiding this comment.
Note: This is a drive-by change. This was needed because we were confusing base and quote in router_usecase.go
PaddyMc
reviewed
May 10, 2024
| @@ -33,9 +33,6 @@ type RoutablePool interface { | |||
| CalculateTokenOutByTokenIn(ctx context.Context, tokenIn sdk.Coin) (sdk.Coin, error) | |||
| ChargeTakerFeeExactIn(tokenIn sdk.Coin) (tokenInAfterFee sdk.Coin) | |||
|
|
|||
| // SetTokenOutDenom sets the token out denom on the routable pool. | |||
| SetTokenOutDenom(tokenOutDenom string) | |||
There was a problem hiding this comment.
Can we remove this at a later date? Will this break the node, if sqsdomain isn't updated?
There was a problem hiding this comment.
No, this will not break the node.
It would only break if we changed serialization.
There was a problem hiding this comment.
Great, happy to approve then
PaddyMc
approved these changes
May 10, 2024
p0mvn
added
A:backport/v25.x
mergify bot
pushed a commit
that referenced
this pull request
May 10, 2024
(cherry picked from commit c1863fd) # Conflicts: # CHANGELOG.md
p0mvn
added a commit
that referenced
this pull request
May 10, 2024
p0mvn
added a commit
that referenced
this pull request
May 10, 2024
* fix: DYDX spot price bug (backport #212) * go mod --------- Co-authored-by: Roman <[email protected]>
cryptomatictrader
added a commit
that referenced
this pull request
May 20, 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 * Code-complete. Need to be tested * Captured Coingecko ID from the asset list * fix: DYDX spot price bug (#212) * changelog & sqsdomain * go mod fix * Removed unused var after running lint * 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 * Changed all mainnet reference to v1 to v2 asset list * Fixed json typo * Fixed test cases: AAVE is no longer unlisted in v2 asset list. Updated tokens.json to include coingecko id * v1 unlisted should be mapped to v2 preview, thus updating the test case * v1 unlisted should be mapped to v2 preview * Removed unused definitions in asset list * Changed reference to is_unlisted to preview in test case --------- Co-authored-by: PaddyMc <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Calvin <[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.
Fixes this:
The core of the bug was that in edge cases the
TokenOutDenomis not set by design. As a result, the quote-based spot price method for Astroport PCL pools would fail, falling back to the CW pool query that would return spot price that is already scaled.That would result in falling back to querying the contract directly but the contract returns already descaled spot price (relative to denom precision by mistake)
Testing
Prod Prices
This PR prices
⏫ Note that all prices are the same but DYDX is fixed