Add multiple price feeds option #65
Conversation
There was a problem hiding this comment.
The changes generally make sense for testing other price feeds.
One issue is that the query is a bit too complicated for me now. (Maybe already was too complicated before.) Since it is essential to the accounting, it has to easy to understand what the query does.
For example, the handling of missing values seems a bit indirect: first, some price feeds are combined and an approximate median is computed. If that is missing, yet another price feed is used. So there seem to be different tiers of price feeds as well. But that is not made explicit anywhere. And only the Dune price feed is used for some things (intrinsic prices, native token prices) that other price feeds could theoretically also be used for.
| @@ -43,6 +82,35 @@ precise_prices as ( | |||
| group by 1, 2, 3 | |||
| ), | |||
|
|
|||
| multiple_price_feeds_pre as ( | |||
There was a problem hiding this comment.
This name does not help me understand what this table means. You can just select from a union in the next table. If it is a conceptually important table to you, the name should be changed.
| hour, | ||
| token_address, | ||
| decimals, | ||
| approx_percentile(price_unit, 0.5) as price_unit, |
There was a problem hiding this comment.
We should document explicitly what this function selects. It can differ from the median, e.g. select approx_percentile(x, 0.5) from (select 1 as x union all select 2 as x) gives 2.
There was a problem hiding this comment.
Switched the code to compute the correct median, based on the No2 solution proposed here
https://medium.com/learning-sql/how-to-calculate-median-the-right-way-in-postgresql-f7b84e9e2df7
| from auction_price_feed | ||
| ), | ||
|
|
||
| multiple_price_feeds as ( |
There was a problem hiding this comment.
I would probably rename to median_price_feed.
| @@ -25,8 +25,47 @@ with token_times as ( | |||
| group by 1, 2 | |||
| ), | |||
|
|
|||
| imported_price_feed as ( | |||
There was a problem hiding this comment.
Please add some comments on what those tables mean, or on the meaning of some tables in the context of the full query.
| @@ -135,5 +187,5 @@ native_token_prices as ( | |||
| ) | |||
|
|
|||
| select * from prices | |||
| union all | |||
| union distinct | |||
There was a problem hiding this comment.
Why was this change required? Before, the entries in the prices table were always distinct from entries in the native_token_prices table since the token address was different. Does this still hold?
If not, there might be additional issues due to a token having multiple prices at the same time, leading to duplicate entries when joining.
There was a problem hiding this comment.
Tbh i didn't think too much about it and it just looked more correct to me (in the sense that if you provide a prices table where native tokens are included, then you should not use the corresponding native_token_prices entries). In the current state, i don't think it can ever happen that the two table have common entries so i will switch back to union all
|
Is there a way to test this query? |
fleupold
added
the
E:9.1 Streamline mainnet payouts process
This PR adds the option to use multiple price feeds as follows:
approx_percentile(0.5)is used to compute a price based on these price feeds. This replaces what has been now only the dune price.In case all price feeds return null, we continue to use the fallbacks that have been used up till now