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

[proposal] sector-level spells redesign #4637

Closed
jeff-dude opened this issue Oct 19, 2023 · 7 comments
Closed

[proposal] sector-level spells redesign #4637

jeff-dude opened this issue Oct 19, 2023 · 7 comments
Labels
dbt: dex covers the DEX dbt subproject dbt: nft covers the NFT dbt subproject dune team created by dune team enhancement New feature or request WIP work in progress

Comments

@jeff-dude
Copy link
Member

jeff-dude commented Oct 19, 2023

overview

note: for the purposes of simplicity, most of this will focus on dex.trades as it's actively starting the redesign, but all sectors apply!

dex.trades has more or less followed the same table design since the early days of abstractions on dune. when spellbook was built, a bit more structure was put in place around how dex.trades is built & how it is orchestrated in production to ensure fresh data for all users. over time, as more and more projects have been added & the tech stack which we use to build the spells has improved, we've found various opportunities for improvement. this redesign effort will look to implement all of these opportunities for an improved spell experience for all.

what's changing (and why)?

  • base project-level spells will only contain data from raw & decoded source tables
    • examples: uniswap_v1_ethereum.base_trades, uniswap_v3_optimism.base_trades
    • to keep dbt lineages & both CI/prod orchestration clean and easy to support
    • less full refreshes of historical data in prod, as data is much less likely to change and/or be modified when reading only raw/decoded data
    • these spells will still be materialized incrementally, same as before
    • the intention isn't for end users to query these on the dune app, but rather as a building block to reach ultimate destination in dex.trades
    • if project-level spells want to be exposed on the dune app to be queried, the recommendation will be to create a spell view that sits on top of dex.trades and filters on blockchain/project/version/whatever is needed
  • chain union spells will contain a union all of input spells, i.e. spells from the previous step on the same blockchain
    • dex_<blockchain>.base_trades — this will allow for another building block towards dex.trades
    • todo: check if these will be exposed or not on the app, likely not will not expose for now
    • these chain union spells will be materialized incrementally (or try a view, if not intended to be queried on the app?) the chain union spells are views
    • chain-level spell views will call a dbt macro, which joins to raw transaction per chain to obtain necessary fields: from, to, index
  • sector-level base trades spell is final stepping stone prior to dex.trades
    • union all on all blockchains
    • materialized incrementally
  • sector-level final union spell will call a dbt macro which enriches the raw/decoded data with all the necessary metadata
    • dex.trades
    • metadata: tokens.erc20, prices.usd, etc.
    • having the metadata limited to the final sector-level spell allows for easier contributions of new projects, simplified prod orchestration & one dbt macro to centralize the logic
  • new standardized dbt macro to assign amount_usd to trades
    • lookup to prices.trusted_tokens to find large mcap tokens with most accurate pricing -- if find a match, use trusted token
    • otherwise, coalesce token_bought, token_sold like before

who is leading this effort?

dune team & resident wizards, along with any helpful ideas/contributions/feedback from the community 🙏

where can the code be found?

there'll be various PRs over time, but two examples:
1 - #4533 for dex.trades redesign effort, starting with _beta until we get all projects migrated to the format -- plz note, this is still WIP!
2 - #4519 for referral.awards new sector led by @0xRobin, who is using the new sector design principles

there have been quite a few PRs already for nft.trades redesign, all work can be found here -- plz note, this is also in _beta phase until all spells in the lineage are formatted to the new design.

final words

we want to add more structure and rigid design to sector-level spells. there are plenty of existing sectors in spellbook, and surely plenty more to come. all sectors should be able to follow similar design principles. once we get these sectors cleaned up, it'll make everyone's lives easier to both contribute & support moving forward.

we can use this issue to continue conversation on sector-level designs, then eventually close and use the learnings to build a robust readme for universal sector spell design, as well as sector-specific readme files to talk through nuances of each.

looking forward to continuing to improve spellbook 🚀

appendix

organizing suggestions from the community in a bullet-point list:

  • dex.trades new columns: block_number, tx_index to help differentiate trades within a transaction, within a block
  • dex.trades if no pricing data available from prices.usd, should we coalesce the field to pull from dex.prices as a backup?
  • all sectors: enforce a relationship test from sector-level spell project column to new <sector>.info spells, which will contain project and various other information to inform users of what the project is, how to find, etc.
    • this will require new base project level spells to also add values to the <sector>.info spells
@jeff-dude jeff-dude added enhancement New feature or request WIP work in progress dune team created by dune team dbt: dex covers the DEX dbt subproject labels Oct 19, 2023
@jeff-dude jeff-dude changed the title [proposal] dex.trades redesign [proposal] sector-level spells redesign Oct 19, 2023
@jeff-dude
Copy link
Member Author

jeff-dude commented Oct 19, 2023

fyi to dune team / RWs: @aalan3 @couralex6 @antonio-mendes @0xRobin @Hosuke

fyi to frequent contributors: @hildobby @MSilb7 @henrystats @tomfutago

edit: tagging more who have contributed
@grkhr @markusbkoch

plz link back to this issue for any conversation that relates to sector-wide designs 🙏

@jeff-dude
Copy link
Member Author

gm gm wizards 🌞

the dex sector redesign foundational PR has been merged #4533 -- thanks again for all the feedback and iterations. specific items from the PR to callout:

  • this foundational PR focused on uniswap dex across all chains, as well as one uniswap fork, which allowed us to leverage a macro approach to the logic to use universally in all spells. this allows for a structure in place to do the same for other protocols which have many forks
  • naming standards
    • there was some back and forth & slight differences in opinion, but ultimately, the majority of these spells are meant for the backend data engineers and not user-facing on the dune app, so we landed on base_ prefix for table aliases to indicate it's a stepping stone to a final trades table
    • similar for the macro name, we landed on using uniswap_ prefix and all that forked can use it
  • directory structure
    • macros/models/_sector/dex/
    • models/_sector/dex/trades/<blockchain>/platforms/
  • materialization strategy
    • platform/project level: incremental
    • blockchain level: view
    • sector level: incremental
  • model config settings
    • always include both schema and alias -- let's avoid adding schema to project file, that'll be cleaned up over time
    • no changes to table configs -- incremental/merge/delta
    • add incremental predicates -- this allows the target to also be filtered on incremental loads to the same as the source
  • macro vs. code in model
    • if standalone dex, more than fine to have code directly in model
    • if forked dex and/or repeatable logic, use or create a macro

next steps (for dex.trades_beta specifically):

  • edit the macro which grabs columns from base transactions table to output additional columns
    • ethereum.transactions.index --> dex_ethereum.base_trades.tx_index and then downstream to dex.trades_beta
  • double check for block_number -- if not, add throughout lineage and join conditions
  • add more & more dexes!
    • please keep one PR <--> one project/protocol for easier review/merge

@jeff-dude
Copy link
Member Author

edit the macro which grabs columns from base transactions table to output additional columns

  • ethereum.transactions.index --> dex_ethereum.base_trades.tx_index and then downstream to dex.trades_beta

@0xRobin curious your thoughts on this macro:
if we add more columns, then should we rename the macro to be more generic? if so, what is the impact to downstream models? i don't want to break too much. it would be nice to pull more columns from this macro to keep things clean.

@0xRobin
Copy link
Collaborator

0xRobin commented Nov 9, 2023

@0xRobin curious your thoughts on this macro:
if we add more columns, then should we rename the macro to be more generic? if so, what is the impact to downstream models? i don't want to break too much. it would be nice to pull more columns from this macro to keep things clean.

I have considered this macro to be very temporary (until these columns are in the source tables, which should be asap).
So either: we add more columns and check CI if anything breaks or we duplicate it to a new version which has more columns and use that where necessary. Both valid options imo. The macro was more for tracking the places we'll need to revisit than it was for general application across spellbook.

--
note: I duplicated the macro in the NFT PR to add an extra data column (highly specific to nft.trades)
https://github.com/duneanalytics/spellbook/blob/cf293da0564cb3d6727efe5d9020a7db9832df8e/macros/models/_sector/nft/add_nft_tx_data.sql

@grkhr
Copy link
Contributor

grkhr commented Nov 9, 2023

@jeff-dude @0xRobin We are planning to reuse add_tx_from_and_to macro as well.

May be it's better to make it more generic?

e.g.:

{% macro add_tx_columns(
    model_cte
    , blockchain
    , columns = ['from', 'to']
    )
%}

select
    model.*
    {% for column in columns %}
    , tx."{{column}}" as tx_{{column}}
    {% endfor %}
from {{model_cte}} model
inner join {{source(blockchain, 'transactions')}} tx
    on model.block_number = tx.block_number
    and model.tx_hash = tx.hash
    {% if is_incremental() %}
    where {{incremental_predicate('tx.block_time')}}
    {% endif %}
{% endmacro %}

@jeff-dude
Copy link
Member Author

jeff-dude commented Nov 9, 2023

@jeff-dude @0xRobin We are planning to reuse add_tx_from_and_to macro as well.

May be it's better to make it more generic?

e.g.:

{% macro add_tx_columns(
    model_cte
    , blockchain
    , columns = ['from', 'to']
    )
%}

select
    model.*
    {% for column in columns %}
    , tx."{{column}}" as tx_{{column}}
    {% endfor %}
from {{model_cte}} model
inner join {{source(blockchain, 'transactions')}} tx
    on model.block_number = tx.block_number
    and model.tx_hash = tx.hash
    {% if is_incremental() %}
    where {{incremental_predicate('tx.block_time')}}
    {% endif %}
{% endmacro %}

let me open a PR to get this moving

PR: #4757

@jeff-dude
Copy link
Member Author

closing in favor of #4759 where we apply to dex sector as first example for all to follow. readme in dex sector directory contains some info on the design & can be modified via PRs any time

@jeff-dude jeff-dude unpinned this issue Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dbt: dex covers the DEX dbt subproject dbt: nft covers the NFT dbt subproject dune team created by dune team enhancement New feature or request WIP work in progress
Projects
None yet
Development

No branches or pull requests

3 participants