Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

explore page: use common numeraire #223

Merged
merged 9 commits into from
Jan 13, 2025
Merged

explore page: use common numeraire #223

merged 9 commits into from
Jan 13, 2025

Conversation

TalDerei
Copy link
Contributor

@TalDerei TalDerei commented Dec 18, 2024

references #188

  • migrate useSummary useQuery into /trade/api,
  • summaries: Implements a left join of the summary with the USDC prices table in the SQL query instead of making multiple internal API server requests to /api/summary, and denominates the trading pair liquidity and volume in USDC,
  • stats: Implements a left join of the summary with the USDC prices table in the SQL query, and denominates the stats in USDC,
  • fixes internal server error in api/stats,
  • filters out pairs with zero liquidity and trading volume,
  • utility: split USDC price calculations into separate utility method

@TalDerei TalDerei self-assigned this Dec 18, 2024
@TalDerei TalDerei marked this pull request as draft December 18, 2024 05:21
queries: quoteAssets.map(asset => ({
queryKey: ['multipleSummaries', asset, 'USDC'] as const,
queryFn: async () => {
return apiFetch<SummaryData | NoSummaryData>('/api/summary', {
Copy link
Contributor Author

@TalDerei TalDerei Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the /api/stats and /api/summaries endpoints from pindexer may be returning incorrect information, impacting metrics like "total trading volume (24h)" and the largest "trading pair (24h volume)" for the USDC/USDY pair.

previous discussions suggest that this may not simply be related to the denom exponents not being handled properly. cc @cronokirby

src/pages/trade/model/useSummary.ts Outdated Show resolved Hide resolved
src/shared/utils/price-conversion.ts Outdated Show resolved Hide resolved
src/pages/explore/ui/pairs.tsx Outdated Show resolved Hide resolved
src/pages/explore/api/use-summaries.ts Show resolved Hide resolved
@TalDerei TalDerei marked this pull request as ready for review December 18, 2024 06:58
Copy link
Contributor

@VanishMax VanishMax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: is it possible to implement USDC price calculation using SQL?

As I understood the code correctly, it firstly calls useSummaries, waits for it to fetch, and then makes multiple requests to /api/summary with USDC as a quote asset. In my tests, these sequential requests run from 1s to 3s. It's not a bad score, but we can sure do better. Plus, I think that making multiple requests (right now 6 /api/summary requests are made in parallel) are highly error prone and not-so-scalable.

We could change this to an improved SQL query. That's just one join with the sub-table where quote assets are USDC and multiplication method. This could also unblock sorting by calculated volume, limit and filter the results correctly.

issue (realized while writing question before): with this solution, we cannot sort the assets by usdc volume. For now, it is sorted by trade amount, which is not as useful for users as volume.

src/pages/explore/ui/pair-card.tsx Outdated Show resolved Hide resolved
src/pages/explore/ui/pair-card.tsx Outdated Show resolved Hide resolved
src/pages/explore/ui/pairs.tsx Outdated Show resolved Hide resolved
src/pages/trade/model/useSummary.ts Outdated Show resolved Hide resolved
src/shared/utils/price-conversion.ts Outdated Show resolved Hide resolved
@TalDerei
Copy link
Contributor Author

TalDerei commented Jan 6, 2025

note to reviewers: I'll be addressing the feedback today after a brief hiatus focusing on mobile work. @erwanor will be investigating the USDY denom coming out of pindexer.

@TalDerei
Copy link
Contributor Author

TalDerei commented Jan 8, 2025

question: is it possible to implement USDC price calculation using SQL?

@VanishMax coming back to this, is this something we still want to try to implementing using SQL, or is the current approach of using useMultipleSummaries sufficient? you mentioned referencing another PR that was doing something similar related to pricing?

I think we can also just iterate on this separately?

@VanishMax
Copy link
Contributor

@TalDerei Here's the code I mentioned before: https://github.com/penumbra-zone/dex-explorer/pull/204/files#diff-389f7431dcbf5738fe1724971d0c7a329fba1bd4b7058c49dd5abbd9d875a5f6R91

I believe it's more efficient and easier to implement – the left join of the summary with usdc prices table.

@cronokirby
Copy link
Contributor

Not a blocker for merging this, but perhaps we should instead add a column to pindexer for doing this in advance.

Comment on lines +54 to 58
// TODO: should this be `direct_volume_over_window`?
let volume = toValueView({
amount: summary.liquidity,
metadata: quoteAsset,
});
Copy link
Contributor Author

@TalDerei TalDerei Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: should this have been summary.direct_volume_over_window rather than summary.liquidity? cc @VanishMax

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought the stats should display the total existing liquidity (that's probably what i'd like to see on such page) but we sure can always change it

/**
* Calculates USDC-normalized amounts for trading pair values (volume or liquidity)
*/
export function calculateEquivalentInUSDC(
Copy link
Contributor Author

@TalDerei TalDerei Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

price calculations utility method

todo: sanity check accuracy of all consumers of calculateEquivalentInUSDC, specifically the values (volume / liquidity) and metadata being passed in.

Comment on lines +56 to +66
// Converts liquidity and trading volume to their equivalent USDC prices if `usdc_price` is available
if (summary.usdc_price && usdc) {
liquidity = calculateEquivalentInUSDC(summary.liquidity, summary.usdc_price, quoteAsset, usdc);

directVolume = calculateEquivalentInUSDC(
summary.direct_volume_over_window,
summary.usdc_price,
quoteAsset,
usdc,
);
}
Copy link
Contributor Author

@TalDerei TalDerei Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: BTC and USDY still have strange denom coming out of pindexer – we should check if penumbra-zone/penumbra#4981 sufficiently addresses this. cc @cronokirby

Screenshot 2025-01-12 at 2 01 37 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointing my local dex-explorer dev env at the prod pindexer db after conor's recent reindexing resolves this

Screenshot 2025-01-13 at 2 59 06 PM

});

// Converts liquidity and trading volume to their equivalent USDC prices if `usdc_price` is available
if (stats.usdc_price && largestPairEnd) {
Copy link
Contributor Author

@TalDerei TalDerei Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: api/stats not displaying correct values currently

Screenshot 2025-01-12 at 2 03 42 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks more normal after recent reindexing

Screenshot 2025-01-13 at 3 00 22 PM

Copy link
Contributor

@VanishMax VanishMax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor non-blocking comments. Feel free to merge

);

// TODO: create `pnum.multiply()` utility: https://github.com/penumbra-zone/dex-explorer/issues/231
const result = liquidityOrVolume * usdcPrice * 10 ** expDiff;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I see @JasonMHasperhoven using .times() function on BigNumber in collab with pnum. Maybe this could help? https://github.com/penumbra-zone/dex-explorer/blob/main/src/pages/trade/model/positions.ts#L285

src/shared/api/server/summary/single.ts Show resolved Hide resolved
src/pages/trade/model/useMarketPrice.ts Show resolved Hide resolved
Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine for now, and seems to work correctly based on running on a new reindex; we should probably move back some of this logic into pindexer, but we can refactor subsequently.

@TalDerei TalDerei merged commit 3687617 into main Jan 13, 2025
3 checks passed
@TalDerei TalDerei deleted the common-numeraire branch January 13, 2025 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants