fix(insights): when feeds aren't trading, show last valid price #3117
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
cprussin
force-pushed
the
cprussin/dont-tick-offline-feeds
branch
from
714ada5 to
f4ac640
Compare
I originally planned to do this in the context but it felt incorrect to me because the context is intended to expose directly the state on chain. It seems more correct that the transformation should be applied where the data is displayed since it's reasonable that we'd want some places to display the data as-is and some to have this transformation -- the |
|
@cprussin i get where you are coming from but still what you have is very error prone. My rule is always how as a reader i understand the code and change it. Am i sure reading this code that all places are handled well? No. If i later change it i can miss one too. I don't get why onchain state should be as is, i see context as an adapter to the code representation and needs. And then i'd define types that represent what i need in the app. If the main raw data can sometimes be used, then make new types of what is used that make this distinction clear within the context. This makes the reasoning obvious too (as it'll have low coupling, just one place that you look to understand the assumptions/..., and another place concerned with display,..) |
| unique.set(a.time, a); | ||
| } | ||
| for (const b of bs) { | ||
| unique.set(b.time as number, b); | ||
| unique.set(b.time, b); |
There was a problem hiding this comment.
Wonder if we need to add some check here so that if the timestamp for a LineData and a WhitespaceData are equal, the LineData wins.
I.e., if we're using 1 day as resolution, and the status goes down and up within that same day, we'll be guaranteed to see the price instead of whitespace.
Perhaps this could/should be done in the Clickhouse query even? Not sure...
3 participants
Summary
When feeds are not currently trading, we would previously still show the price point published to pythnet. However this is misleading in IH since it looks like the feed is still live and updating. With this PR, we now only show ticking prices when feeds are trading -- when they aren't we instead fall back to
previousPrice/previousTimestamp.We also enter whitespace data into the chart for points where the feed is not trading -- to be honest there are probably more intuitive ways to account for this in the chart (e.g. maybe we should just skip these points and default the chart to show a window including the last published point or something), but this is a reasonable stopgap that correctly displays what we saw in pythnet, while removing confusion around stale data looking like it's still being published. cc @fhqvst , this is something to think of in future iterations of the chart.
Rationale
Resolves some confusing behavior of IH
How has this been tested?