-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add main slippage query #20
Conversation
I am not sure how to fix the sqlfluff errors around the use of the dictionary from the |
cowprotocol/accounting/rewards/mainnet/excluded_batches_query_3490353.sql
Outdated
Show resolved
Hide resolved
pre_batch_transfers as ( | ||
select * from all_transfers_temp | ||
order by tx_hash | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an execution speed optimization or an artifact from removing parts of the query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Artifact of removing part of the query. The union above was written as
select * from (select * from .... union select * from ....) as _ order by tx_hash
and sqlfluff was complaining about the underscore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it can be removed and the table all_transfers_temp
can be used directly below.
all_transfers_temp as ( | ||
select * from user_in | ||
union all | ||
select * from user_out | ||
union all | ||
select * from other_transfers | ||
union all | ||
select * from eth_transfers | ||
union all | ||
select * from sdai_deposit_withdrawal_transfers | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it (and lots of the code above) could be replaced by
select
tx_hash,
sender,
receiver,
token_address as token
from "query_4021257(blockchain='ethereum',start_time='{{start_time}}',end_time='{{end_time}}')"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was not at all aware of this query. I will need to look at it and see what i can do
incoming_and_outgoing as ( | ||
select | ||
block_time, | ||
tx_hash, | ||
solver_address, | ||
symbol, | ||
token, | ||
amount, | ||
transfer_type | ||
from incoming_and_outgoing_temp | ||
order by block_time | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me this table should be the result of an independent query. It looks super close to what query 4021257 does, with a sprinkle of joining on batch and erc20 data and adding a sign.
What is the use of joining with additional data here?
cowprotocol/accounting/rewards/mainnet/processed_token_imbalances_4057345.sql
Outdated
Show resolved
Hide resolved
incoming_and_outgoing_final as ( | ||
select | ||
block_time, | ||
tx_hash, | ||
solver_address, | ||
symbol, | ||
amount, | ||
transfer_type, | ||
case | ||
when token = 0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee then 0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2 | ||
else token | ||
end as token | ||
from incoming_and_outgoing_premerge | ||
order by block_time | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This table could also just have columns block_time, tx_hash, token, amount(, transfer_type for debugging). It could be constructed from the result of two queries:
- token imbalances; where transfer_type in ('user_in', 'user_out', 'amm_in', 'amm_out')
- fees; where transfer_type in ('protocol_fee', 'network_fee'(, 'partner_fee'))
token, | ||
tx_hash, | ||
sum(amount) as token_imbalance_wei, | ||
date_trunc('hour', block_time) as hour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truncating to hour here seems strange. The final token balance sheet might as well only have columns block_time, tx_hash, token, amount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah definitely a good point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the truncation for now
prices.usd | ||
inner join token_times | ||
on | ||
minute between date(hour) and date(hour) + interval '1' day -- query execution speed optimization since minute is indexed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not thing this table has an index (anymore?).
cowprotocol/accounting/rewards/mainnet/slippage_query_3427730.sql
Outdated
Show resolved
Hide resolved
concat(environment, '-', name) | ||
) | ||
|
||
select * from {{cte_name}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this parameter seems unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is so that we can also see the results_per_tx
table, if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the results per transaction table should probably be its own query. or rather there should be a query with all balances changes (also those which cancel) and prices.
the aggregation and printing of links seems conceptually different. the filtering on excluded batches should also probably happen between results_per_tx
and results
as it is mostly related to prices and not to missing information on imbalances.
if those queries are split, the first query could also be used to compute unusual slippage settlements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some changes to address some of these.
@@ -0,0 +1,162 @@ | |||
-- https://github.com/cowprotocol/solver-rewards/pull/342 | |||
with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there cannot be a new line after with (unless it is a comment)
cowprotocol/accounting/rewards/mainnet/processed_token_imbalances_4057345.sql
Outdated
Show resolved
Hide resolved
One alternative approach would be to have a query like this with a balance sheet per transaction per token with prices. This is typically the level I am looking at when debugging slippage. (Sometimes I go down one level deeper and want to see classified balance changes with prices.) That can then be aggregated into values per batch and values per solver. |
d4b692a
to
af42aa4
Compare
The latest version was tested and is able to give identical results to the current query for the accounting week of Sep3-10, 2024. I am taking the libery to merge this as we need to have a working version for tomorrow's payouts. And i would suggest a new PR that properly restructures slippage accounting |
This PR splits the main slippage accounting query into two queries:
The excluded batches query (3490353) was already a separate query, which we now put here under version control