Conversation
|
Are you sure you want to use the legacy Resolved |
fhenneke
left a comment
fhenneke
left a comment
There was a problem hiding this comment.
This looks like a better version of what we currently use for payments. It also introduces lots of duplicate code, which is not a straight copy. That will make it difficult to maintain.
I left some comments, only some of them actionable.
The way I reviewed this by comparing the two files solver_rewards_per_block_5534936.sql and main_rewards_dashboard_query_2510345.sql. All changes look reasonable. For some of the comments I would like to hear the reasoning for why it was changed. As I mentioned above, overall the query looks way better.
One thing one would have to do, probably, is to check aggregated results of this query against our main dashboard query.
Is it realistic to switch our main dashboard to use this query and just aggregate? Then there would be only one query to maintain, we would use the better query right away for payments, and data for finance is always in sync with payments.
| select | ||
| solver, | ||
| block_time, | ||
| least({{quote_reward}}, {{quote_cap_native_token}}) * count(1) as quote_reward_native |
There was a problem hiding this comment.
This week the cap for quote rewards has changed. We might need to store values for all chains and have a cut-over block, if this query is supposed to work long term and across accounting periods.
| left join {{blockchain}}.blocks b | ||
| on ad.block_deadline = b.number | ||
| ) | ||
| , auction_data_in_native as ( |
There was a problem hiding this comment.
The original query uses query_4842868 to filter for auctions which are not rewarded. That would be required here as well, if this is supposed to be accurate.
There was a problem hiding this comment.
I couldn't find that filter in https://dune.com/queries/2510345... where should it be?
There was a problem hiding this comment.
Oh, you are correct. I missed that that code in https://github.com/cowprotocol/dune-queries/blob/0c42324bca19ade690e3c47043d766ea8979438f/cowprotocol/accounting/rewards/main_rewards_dashboard_query_2510345.sql#L37C1-L46C6 is commented out. The query on dune has that code removed. (We should make sure our queries on dune are exactly identical to what we have in this repo.)
More abstractly, this shows that having a query work across time is not straight forward. We used that feature only once. But using your query on that particular week will immediately produce different results.
| from order_quotes as oq | ||
| inner join cow_protocol_{{blockchain}}.trades as t | ||
| on oq.order_uid = t.order_uid | ||
| and oq.quote_solver != 0x0000000000000000000000000000000000000000 |
There was a problem hiding this comment.
(Not a comment on the PR, but to give some historic context: when we introduced the quote competition, we used a dummy address for something. That had to be taken care of initially. By now, that is probably not required anymore. But we do not have efficient means to check correctness of our accounting scripts. Therefore we keep such things around.)
Made some modifications according to a couple of comments from @fhenneke to make use of the newer materialized tables being used in the current main rewards query
fixed mistake with new price conversion code
improved quote cap logic
|
Split the logic into 2 parts: one auction-based and another payout week-based. |
2 participants
Finance has requested a more granular view on solver rewards. This replicates the existing logic seen in main_rewards_dashboard_query_2510345