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

Refactor rollup_economics spell #6426

Merged
merged 34 commits into from
Aug 20, 2024

Conversation

lgingerich
Copy link
Contributor

@lgingerich lgingerich commented Jul 24, 2024

Background

The rollup_economics spell has become quite computationally heavy such that the CI cluster often runs out of memory and crashes. This PR is to attempt a refactor similar to other spellbook designs and get it to a more future-proof design.

The main PR discussing optimizations of this spell is #6169, and lesser so #6247.

Changes

The rollup_economics spell includes four types of fee categorizations: l2_revenue, l1_blob_fees, l1_data_fees, and l1_verification_fees.

l2_revenue and l1_blob_fees: These pull data from already aggregated upstream sources so can be easily handled in a single file. I have another open PR to refactor l2_revenue (#6408) and I've mirrored that logic here. That PR is ready to merge and I'll reconcile here once it is.

l1_data_fees and l1_verification_fees: These are the main ones causing heavy load and is what I focused the refactoring on. I tried to mirror the structure of dex.trades. For each of these, I've created a single sql file for each chain, then in a downstream model, aggregate all chains together and enrich with USD pricing. This fixes the repeated joins on prices.usd. It doesn't reduce any queries of ethereum.transactions but does at least split into multiple files. A table is materialized for each chain-level implementation (e.g. rollup_economics_zksync_l1_data_fees, rollup_economics_linea_l1_data_fees, etc.).

Notes:

  • l1_blob_fees doesn't necessarily need to be in its own folder since it's just a single sql file, but since it's at the same hierarchy in the data lineage as l1_data_fees and l1_verification_fees, I thought it made sense to mirror those setups.
  • I've only included linea and zksync for now, and once we agree on a structure to move forward with, I will move over the rest of the chains.
  • RE: blockchain naming. l1_blob_fees pulls from an upstream source with human readable chain names which differ from the standard Dune naming (i.e. should be "zksync", not "zkSync Era"). I have not made the change yet but I would prefer to move to the standard Dune naming model for each of these chains, and potentially change the name column to be blockchain. Let's discuss this further.
  • Some individual chains need some additional refactoring (i.e. likely Arbitrum) but we can save that for after this main refactoring is done.

@jeff-dude @0xRobin @Hosuke @Jam516 I would appreciate comments/critiques from all of you

@dune-eng
Copy link

Workflow run id 10084493257 approved.

@dune-eng
Copy link

Workflow run id 10084493381 approved.

@dune-eng
Copy link

Workflow run id 10085945308 approved.

@dune-eng
Copy link

Workflow run id 10085945661 approved.

@dune-eng
Copy link

Workflow run id 10086039523 approved.

@dune-eng
Copy link

Workflow run id 10086039673 approved.

@dune-eng
Copy link

Workflow run id 10086158665 approved.

@dune-eng
Copy link

Workflow run id 10086158790 approved.

@dune-eng
Copy link

Workflow run id 10086243299 approved.

@dune-eng
Copy link

Workflow run id 10086243545 approved.

@dune-eng
Copy link

Workflow run id 10086394455 approved.

@dune-eng
Copy link

Workflow run id 10086395005 approved.

@jeff-dude jeff-dude requested a review from 0xRobin July 29, 2024 20:41
@jeff-dude jeff-dude added the ready-for-review this PR development is complete, please review label Jul 29, 2024
@dune-eng
Copy link

Workflow run id 10151783462 approved.

@dune-eng
Copy link

Workflow run id 10151783278 approved.

@0xRobin 0xRobin self-assigned this Jul 30, 2024
Copy link
Collaborator

@0xRobin 0xRobin left a comment

Choose a reason for hiding this comment

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

Overall this looks very good! ✔️

Can we try to have these as views:

  • rollup_economics_l1_data_fees.sql
  • rollup_economics_l1_verification_fees.sql

I don't expect these to be queried directly, and the aggregated data is present in l1_fees. Trying to reduce the amount we're storing the same data multiple times across models.

RE: blockchain naming. l1_blob_fees pulls from an upstream source with human readable chain names which differ from the standard Dune naming (i.e. should be "zksync", not "zkSync Era"). I have not made the change yet but I would prefer to move to the standard Dune naming model for each of these chains, and potentially change the name column to be blockchain. Let's discuss this further.

I agree on this but would make the changes in a follow up PR to keep the refactor clean.

@lgingerich
Copy link
Contributor Author

@0xRobin

  • rollup_economics_l1_data_fees.sql
  • rollup_economics_l1_verification_fees.sql

In the existing model, these are both set as incremental runs. I'm fine changing to views but just want to make sure you're good changing this behavior.

RE: naming — sounds good will leave for a later PR

@0xRobin
Copy link
Collaborator

0xRobin commented Aug 6, 2024

@jeff-dude I'm still trying to understand how to handle sources in this new format. How should I fix this?

@lgingerich to prevent having this issue of double source definitions we've come to the following standard:
Any dependencies between different dbt subprojects can be listed as am "output" with the project that it's build in.
For this case, gas.fees resides in daily_spellbook so you should keep the source definition in _subprojects_outputs/daily_spellbook and remove the one in _subprojects_outputs/hourly_spellbook

@dune-eng
Copy link

dune-eng commented Aug 6, 2024

Workflow run id 10268546895 approved.

@dune-eng
Copy link

dune-eng commented Aug 6, 2024

Workflow run id 10268546538 approved.

@jeff-dude jeff-dude self-assigned this Aug 7, 2024
@jeff-dude jeff-dude added in review Assignee is currently reviewing the PR and removed WIP work in progress labels Aug 7, 2024
@jeff-dude
Copy link
Member

there seems to be some inconsistencies with usage of rollup_economics and rollup_economics_ethereum in terms of schema names / corresponding file names. all spells fall under ../rollup_economics/ethereum/ directory path, so i'd imagine having _ethereum in names is expected?

changing schema names can cause some impact downstream, so likely fine to leave as-is, but wanted to call this out

@jeff-dude jeff-dude added ready-for-merging and removed in review Assignee is currently reviewing the PR labels Aug 19, 2024
@lgingerich
Copy link
Contributor Author

@jeff-dude good callout, fixed now

@dune-eng
Copy link

Workflow run id 10459743612 approved.

@dune-eng
Copy link

Workflow run id 10459743847 approved.

@dune-eng
Copy link

Workflow run id 10459821152 approved.

@dune-eng
Copy link

Workflow run id 10459821145 approved.

@dune-eng
Copy link

Workflow run id 10459845066 approved.

@dune-eng
Copy link

Workflow run id 10459845300 approved.

@jeff-dude jeff-dude merged commit 9e76155 into duneanalytics:main Aug 20, 2024
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants