nft.trades redesign open discussion

[jeff-dude]

who

efforts led by resident wizard @0xRobin with support from dune team & community members. anyone is welcome to participate in the design discussions here 🫂

what

redesign nft.trades to be more efficient operationally & easier for wizards to contribute new code.

when

actively happening now, under the _beta naming standards, to keep separate from nft.trades while under development and testing.

where

right here in the spellbook repo!
initial PR: #3129
plenty of info between the PR description & various comments, along with the migration markdown file included -- and of course, plenty in the code itself.
there are various follow-up PRs to continually fit more marketplaces into the structure, such as:

• adding element marketplace: Restructure Element to new NFT trades setup #3304
• adding / enhancing tests on the sector spell: Source rowcount generic test #3279
• large overhaul (still open): Big NFT sector cleanup! #3253
• blur marketplace: Restructure Blur to new NFT trades setup #3284
• cryptopunks marketplace: Restructure Cryptopunks to new NFT trades setup #3286
• x2y2 marketplace: Restructure x2y2 to new NFT trades setup #3314
  ... more to come!

why

there are a variety of reasons for the redesign. to name a few:

• operational burden on maintaining the spell & it's data quality -- i.e. if a new token is added to tokens_ethereum.erc20, since all base models join to it and pull metadata columns, a full refresh is required on historical trades. this leads to long run times and heavy compute. this is an unneccessary step, as we can keep the base models to just base source tables (decoded & base tables), then join to metadata tables once in the sector-wide spell
• standardize values in hardcoded fields -- i.e. lower/upper case, how to track versions (1 vs. v1), accepted values per column test(s), and more
• universal unique keys for all marketplaces 🙌
• simplify wizards contributions to spellbook -- now all that is needed is the decoded table / base table entry -- the rest is covered and standardized
• ...and more to come

tl;dr

with all the above said, we'd like to open up discussion here for wizards to help us shape the future of nft.trades

• what would you like to see?
• what are pain points when querying on the dune app?
• anything else that comes to mind

initial tags: @0xRobin @Hosuke @0xBoxer @andrewhong5297 @augustog @couralex6 @MSilb7 @chuxinh @hildobby
feel free to bring more in on the conversation

thanks again to rob for kicking this off 🧙

[MSilb7]

Makes sense. I think pushing token and price metadata to the final table should be replicated for dex.trades and perpetual.trades as well!

[jeff-dude]

other sectors will follow similar patterns 🤝

[0xRobin]

Here's an overview of the proposed model setup.

[andrewhong5297]

nice! noticing that nft_trades here is still a view. So is the idea that all the chain top level trades getting materialized will be good enough?

thanks for putting this together.

[0xRobin]

yeah, I would assume that. If not we can always flip the switch and materialize the top level

[jeff-dude]

fyi @dot2dotseurat (as performance came up yesterday in your questions), we're tracking here and open to change as needed

[hildobby]

Very cool guys, ty for the rework!

In order to avoid the need for those reworks going forward, it might be worth defining some guidelines when submitting PRs for spellbook to get tables that are consistent across the board, no?

some examples:

• all spells need to have block_time & block_number when possible, not only one of the two, and not evt_block_time or call_block_time or any other names
• macro table depends on chain specific ones, chain-specific should rarely ever be omitted
• enforce when to capitalise vs lowercase strings. The version column of dex.trades is a mess rn for example, and nft.trades has lowercased projects, while aggregators are capitalised (I probably started those but I think I was told to do so, the inconsistency never made sense to me)

I've contributed to spellbook a decent amount and still find myself clueless extremely often on the structure that I should use

[0xRobin]

💯 more structure and guidance is definitely needed.
I think going forward we're definitely going to push towards all lowercase string fields.

[0xRobin]

we're just a bit hesitant on changing existing ones as user queries might contain equality checks with capitalized strings..

[jeff-dude]

ideally, once we reach final stages on nft specifically, we will rework and/or add readme(s) to provide guidance on new sectors and such. it'd also be good to update the PR description template to simplify and link to readme(s), the current template may be too long and leads to less reading