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

Big NFT sector cleanup! #3253

Merged
merged 23 commits into from
May 17, 2023
Merged

Conversation

0xRobin
Copy link
Collaborator

@0xRobin 0xRobin commented Apr 28, 2023

This PR cleans up the NFT trades sub sector spells.

Baiscally it reduces every platform specific models to only the "events" models, removing the "trades", "fees", "burns", "mints" models which should all just be a view on the "events" model.

We had some inconsistencies with platforms only implementing some of these tables which resulted in platforms being present in events but not in trades or fees,..

New setup:

  • Every platform implements just the "events" model
  • All platforms are combined in the nft_events model
  • 'nft_trades', nft_fees, nft_burns and 'nft_mints' are views on top of nft_events

This cleanup will also help a lot during the ongoing NFT restructuring migration.

There's a small impact on users as models like blur.fees don't exist anymore after this is merged and they should use blur.eventsor the overall nft.fees model.

Part 2:

  1. Moved all the "old" (but still in use!) models under the '_sector/nft` directory structure.
  2. added explicit schema's to those models
  3. removed the expose_spel from those marketplace events models
  4. split any marketplace union views in just the seperate version models (except opensea, that's a mess)
  5. cleaned up dbt_project
  6. introduces <marketplace>.trades view, which is the only marketplace specific view that will get exposed on the App

@0xRobin 0xRobin added dune team created by dune team dbt: nft covers the NFT dbt subproject labels Apr 28, 2023
@0xRobin
Copy link
Collaborator Author

0xRobin commented May 1, 2023

Here's a comparison between the production tables and the tables in the PR to confirm I've not forgotten any specific platform.
https://dune.com/queries/2424413
image

@0xRobin
Copy link
Collaborator Author

0xRobin commented May 1, 2023

✔️ CI checks are actually passing.
Only checks not passing are DuneSQL checks for models that are currently already hidden in Trino.

@0xRobin 0xRobin marked this pull request as ready for review May 1, 2023 13:20
@0xRobin 0xRobin added the ready-for-review this PR development is complete, please review label May 1, 2023
@0xRobin 0xRobin requested a review from jeff-dude May 1, 2023 13:26
@0xRobin 0xRobin mentioned this pull request May 1, 2023
@jeff-dude jeff-dude self-assigned this May 1, 2023
@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 May 1, 2023
@0xRobin 0xRobin requested a review from augustog May 4, 2023 14:12
@0xRobin
Copy link
Collaborator Author

0xRobin commented May 12, 2023

✔️ checks are passing expect dunesql checks on big models due to java heap space.

@jeff-dude
Copy link
Member

can you fix merge conflicts and let CI run again?

i gave a passthrough of the code, i think i follow the general flow:

  • cleanup project file by removing directories which are being moved into the _sector directory, continuing cleanup we've discussed
  • change nft.events to be view on top of all the "old" events base models, which were moved into new directory structure and clearly labeled old
  • remove old base models from data explorer
  • create platform-specific views on top of existing nft.trades spell
  • nft trades/burns/fees also views, similar to events, kept to sector-level
    (follow up: but why is fees/trades on nft_events while burns is on nft_events_old?)
    (also, is mints missing?)
  • remove and/or edit all the old directories that fed nft sector
  • clean up seeds
  • clean up tests

@0xRobin
Copy link
Collaborator Author

0xRobin commented May 17, 2023

@jeff-dude
on mints:
We'll need to put some more thought into the mints structure, something I plan to do after we've finished all of nft.trades, so I've left it in the original directory.
on burns:
This is a special one, it only has data (4455 records) from nftb_bnb_events, so it's not really of any use in the current state.
I'll want to either remove it or replace it by a view on nft.transfers to the burn addresses. Again something for after all of nft.trades

@0xRobin 0xRobin force-pushed the nft-trade-fees-cleanup branch from b400eed to de39fc0 Compare May 17, 2023 08:40
Copy link
Member

@jeff-dude jeff-dude left a comment

Choose a reason for hiding this comment

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

lets push this and only this through, let it ride in prod
edit: canceled the ci test

@jeff-dude jeff-dude merged commit 3ffc84a into duneanalytics:main May 17, 2023
@jeff-dude jeff-dude removed the in review Assignee is currently reviewing the PR label May 17, 2023
@0xRobin
Copy link
Collaborator Author

0xRobin commented May 17, 2023

awesome, I'll monitor the next hourly run 👍

@0xRobin 0xRobin deleted the nft-trade-fees-cleanup branch July 4, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dbt: nft covers the NFT dbt subproject dune team created by dune team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants