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.

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

DUX-823 Prices v2 rework #7391

Open
wants to merge 44 commits into
base: main
Choose a base branch
from
Open

DUX-823 Prices v2 rework #7391

wants to merge 44 commits into from

Conversation

0xRobin
Copy link
Collaborator

@0xRobin 0xRobin commented Dec 27, 2024

Thank you for contributing to Spellbook 🪄

Please open the PR in draft and mark as ready when you want to request a review.

Description:

  • new upstream models for the sqlmesh prices pipeline.

Goal is to:

  • simplify the lineage and bring the vwap calculation into spellbook
  • have only one model flowing into sqlmesh (that has sparse minute price data)

This should resolve any duplicates we're currently having and will allow us to switch tokens from different sources without having temporary gaps or duplicates.

Also resolves DUX-702 while we're at it.


quick links for more information:

@github-actions github-actions bot added WIP work in progress dbt: tokens covers the Tokens dbt subproject labels Dec 27, 2024
@0xRobin 0xRobin marked this pull request as ready for review December 27, 2024 14:07
@github-actions github-actions bot added ready-for-review this PR development is complete, please review and removed WIP work in progress labels Dec 27, 2024
@0xRobin 0xRobin requested review from aalan3 and jeff-dude January 6, 2025 10:18
@0xRobin 0xRobin changed the title Prices v2 rework DUX-823 Prices v2 rework Jan 6, 2025
@0xRobin 0xRobin added the dune team created by dune team label Jan 6, 2025
Copy link
Contributor

@aalan3 aalan3 left a comment

Choose a reason for hiding this comment

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

Had a few comments but lgtm.

@@ -0,0 +1,23 @@
{{ config(
schema='prices_v2',
alias = 'latest',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this model needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not needed for the sqlmesh side, just thought it would be cheaper do do it directly on the sparse dataset then later on the filled one.

SELECT
*
FROM {{ ref('prices_v2_dex_minute_raw') }}
INNER JOIN {{ ref('prices_v2_dex_filter') }} using (blockchain, contract_address)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we do this filtering below in prices_v2_dex_minute_raw directly instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opted to keep prices_v2_dex_minute_raw a clean union model

@jeff-dude
Copy link
Member

is the intention of this PR to remove the need for dex_prices_block altogether? hypothetical steps:

  • merge this PR, build tables in prod
  • update prices pipeline to use new output from here, simplify logic in pipeline downstream
  • remove dex_prices_block from spellbook

if that is the case, where is the logic here for obtaining median price per token per block? did we decide to do away with that logic (that could change results a lot and require more testing)? we would need to perform this calculation prior to the vwap calculation.
https://github.com/duneanalytics/spellbook/blob/main/dbt_subprojects/dex/models/prices/dex_prices_block.sql#L169-L190

@jeff-dude jeff-dude self-assigned this Jan 6, 2025
@jeff-dude jeff-dude added in review Assignee is currently reviewing the PR and removed ready-for-review this PR development is complete, please review labels Jan 6, 2025
@0xRobin
Copy link
Collaborator Author

0xRobin commented Jan 6, 2025

is the intention of this PR to remove the need for dex_prices_block altogether?

That's indeed the end goal.
With the thought also being that we need to do this in parallel to the current running setup to not disturb that.

Do you know why we needed the median block price before doing the vwap?
I don't think median block with weighted average minute on top makes a ton of sense, but I might be missing something.

Is it outliers in dex.trades, where we don't want them to influence the vwap price?
Maybe just doing the median aggregation on the minute level is a good approach to get the same result.

@jeff-dude
Copy link
Member

Do you know why we needed the median block price before doing the vwap? I don't think median block with weighted average minute on top makes a ton of sense, but I might be missing something.

Is it outliers in dex.trades, where we don't want them to influence the vwap price? Maybe just doing the median aggregation on the minute level is a good approach to get the same result.

yeah, we use the median price per block to remove outliers from DEX activity such as MEV/sandwiches/other approaches i'm no expert in 😅 we had hit a lot of outliers across various tokens and this helped get around most. we still have some outlier prices that get through, which will require more in-depth filtering/logic if fixing.

it's documented a bit here:
https://docs.dune.com/data-catalog/curated/prices/overview#methodology

i think we should stick to block for now, and can revisit minute level later to see how it compares. i think the decision of block vs. minute was due to grouping of tx's that could produce those outlier numbers (mev/sandwich/etc)

@0xRobin
Copy link
Collaborator Author

0xRobin commented Jan 7, 2025

Ran some tests, and results look good.
We can safely change to a minute base median methodology instead of block median + minute vwap.
image

@jeff-dude
Copy link
Member

Ran some tests, and results look good. We can safely change to a minute base median methodology instead of block median + minute vwap. image

nice 🙏 as we mentioned, let's get the team on the same page before finalizing plan here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dbt: tokens covers the Tokens dbt subproject dune team created by dune team in review Assignee is currently reviewing the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants