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.

Sign up for GitHub

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

Show last chart data as non-hover price on token info page #4016

Open
wants to merge 1 commit into
base: stage
Choose a base branch
from
Open

Show last chart data as non-hover price on token info page #4016

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions packages/web/components/pages/asset-info-page/chart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,7 @@ const ChartHeader = observer(() => {
data = decHoverPrice;
} else {
if (assetInfoConfig.dataType === "price") {
data =
marketAsset?.currentPrice?.toDec() ??
new Dec(assetInfoConfig.lastChartData?.close ?? 0);
data = new Dec(assetInfoConfig.lastChartData?.close ?? 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have we verified whether this price reflects the most current value? I’m concerned that discrepancies might appear throughout the app

Copy link
Collaborator

@EnricoBarbieri1997 EnricoBarbieri1997 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The difference between the two is noticeable but small.
There has been a discussion going on with Numia talking about that. As of now data from Numia and SQS differ so the last point on the chart and the price displayed when no hover is performed are different and inconsistent. They tried to make the SQS value the same as Numia but they ultimately decided to just use the Numia value in this page

Copy link
Collaborator

@JoseRFelix JoseRFelix Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We should investigate this further because the currentPrice is sourced from Numia instead of SQS. Changing this might introduce risk since more features in the DEX rely on the currentPrice rather than the last chart value. I’d like to avoid potential discrepancies, especially since we recently resolved a related bug in #4015

Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @niccoloraspa

Copy link

@alvarogar4 alvarogar4 Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hey! The discussion mentioned above was related to the portfolio page, but the problem seems to be similar in this case

However, there's a small but important difference:

  • In the portfolio endpoint the last value comes from the LCD endpoints to ensure real time values
  • In the asset price charts, the last datapoint comes from our database because its computationally heavy to do it on request, so with the current state we can't assure fully real time data. We could potentially try to find a solution if you really need it (or at least try to reduce the delays as much as possible)

The delays overall shouldn't be too long in any case, but I just want to make sure we are all aligned before pushing anything to prod.

Let me know if you want to explore any potential improvements

} else {
data = marketAsset?.volume24h?.toDec() ?? new Dec(0);
}
Expand All @@ -201,7 +199,6 @@ const ChartHeader = observer(() => {
assetInfoConfig.hoverData,
assetInfoConfig.dataType,
assetInfoConfig.lastChartData?.close,
marketAsset?.currentPrice,
marketAsset?.volume24h,
]);

Expand Down
Loading