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

Add relationships tests to ensure all nft spells project values are in nft.marketplaces_info #4594

Conversation

hildobby
Copy link
Collaborator

…info

Thank you for contributing to Spellbook!

Please refer to the top of the readme in the root of Spellbook to learn how to contribute to Spellbook on DuneSQL.

@hildobby hildobby added WIP work in progress dbt: nft covers the NFT dbt subproject labels Oct 13, 2023
@hildobby hildobby added ready-for-review this PR development is complete, please review and removed WIP work in progress labels Oct 13, 2023
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.

if i'm following, the intention is for any new nft marketplace added to nft.trades lineage needs to also add that project to this file?

what's the ultimate end goal here between these spells and the test?

@0xRobin what do you think about this test?

@jeff-dude jeff-dude added question Further information is requested in review Assignee is currently reviewing the PR and removed ready-for-review this PR development is complete, please review labels Oct 16, 2023
@jeff-dude jeff-dude requested a review from 0xRobin October 16, 2023 21:36
@hildobby
Copy link
Collaborator Author

@jeff-dude @0xRobin I thought PRs would be the easiest to get the conversation going with issues still completely inactive

The goal with those would be to hold high level info for its sector. Those can be joined on other spells using the project column to get extra info. For now I just added some columns but more can always be added. They provide extra context that isn't necessarily available when looking at the other spells on their own. I definitely don't know the 50+ projects in there so it's nice to easily check.

Other columns could be added such as:

  • has_token (& token_contract)
  • documentation_link

If you disagree with the need for projects to be added here when added to other spells we can remove the test ofc but imo the person adding the project should be the most knowledgeable on those high level easy question and it only takes a minute. What do you guys think?

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.

thanks for the write up in #4631!

I'm a fan of this as it ensure we don't have metadata-tables that become out of sync (and thus not useful) when the reference tables change.
The test is also really fast so there's no real CI overhead added. 👍

@jeff-dude
Copy link
Member

i would suggest for now, we leave out the test, but maintain <sector>.info spells to prepare. when we finalize the nft.trades redesign, we can add this test

i've added it in the appendix section of issue #4637

@0xRobin 0xRobin added blocked and removed question Further information is requested in review Assignee is currently reviewing the PR labels Oct 20, 2023
@hildobby
Copy link
Collaborator Author

Closing, will reopen once the nft sector redesign is done

@hildobby hildobby closed this Oct 22, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked dbt: nft covers the NFT dbt subproject
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants